feat: multimodal embeddings#2559
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors embeddings from one-of input fields to a typed Contents-based multimodal schema, adds EmbeddingInput validation and BatchEmbedding, updates provider converters/routing to support multimodal inputs and typed EmbeddingsByType outputs, and propagates changes through transports, tests, logging, tracing, caching, DB migrations, and UI. ChangesMultimodal Embeddings (single cohesive DAG)
Sequence DiagramsequenceDiagram
participant Client as Client
participant Transport as HTTP Transport
participant Bifrost as Bifrost
participant Schema as Schema Validator
participant ProviderConv as Provider Converter
participant ProviderAPI as Provider API
Client->>Transport: POST /v2/embed or /v1/embeddings/batch (EmbeddingInput.Contents)
Transport->>Bifrost: Forward EmbeddingRequest/BatchEmbeddingRequest
Bifrost->>Schema: req.Input.Validate()
alt validation fails
Schema-->>Bifrost: Validation error
Bifrost-->>Transport: Error (BifrostError)
Transport-->>Client: 4xx/Error
else validation passes
Schema-->>Bifrost: Valid
Bifrost->>ProviderConv: Convert to provider format (may error)
alt conversion fails
ProviderConv-->>Bifrost: Conversion error
Bifrost-->>Transport: Error response
Transport-->>Client: 4xx/Error
else conversion succeeds
ProviderConv->>ProviderAPI: Send provider request (text/image/video...)
ProviderAPI-->>ProviderConv: Provider response (typed embeddings)
ProviderConv->>Bifrost: Convert → BifrostEmbeddingResponse
Bifrost-->>Transport: 200 OK + embeddings
Transport-->>Client: 200 OK
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Confidence Score: 3/5Two defects remain that affect live request paths: a nil-wrapped interface in the Vertex text-embedding converter and silent data loss in the Cohere multi-image converter. The Vertex text-embedding path returns a typed-nil on any non-text part, which the caller wraps in a non-nil interface. Under passthrough-extra-params mode this panics; otherwise it silently sends core/providers/vertex/embedding.go (ToVertexEmbeddingRequest nil return) and core/providers/cohere/embedding.go (multi-image data loss in ToBifrostEmbeddingRequest). Important Files Changed
Reviews (6): Last reviewed commit: "feat: multimodal embeddings" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/bifrost.go (1)
911-940:⚠️ Potential issue | 🟠 MajorPreserve legacy embedding inputs before enforcing
Contents.This now rejects direct callers that still populate legacy embedding fields (
Input.Text,Input.Texts,Input.Embedding,Input.Embeddings), because the empty-Contentsguard runs before any compatibility normalization andValidate()immediately assumesContentsis already populated. That breaks the stack’s stated backward-compatibility contract for embedding requests. Please normalize legacy fields intoContentsbefore this check, or keep the legacy-field presence check here until that conversion has run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 911 - 940, The empty-Contents guard in the embedding request path rejects legacy callers because legacy fields (req.Input.Text, req.Input.Texts, req.Input.Embedding, req.Input.Embeddings) are not normalized to req.Input.Contents before the check; move or call the compatibility normalization that populates req.Input.Contents for legacy fields before the if that checks (req.Input == nil || len(req.Input.Contents) == 0) and isLargePayloadPassthrough(ctx), or alternatively change that guard to also accept legacy-field presence and run the normalization immediately prior to calling req.Input.Validate(); ensure Validate() continues to run against the normalized Contents so legacy callers remain supported.
🧹 Nitpick comments (9)
core/schemas/embedding_multimodal_test.go (1)
9-27: Add one happy-path validation case here.These tests only pin failure paths. If
Validate()regressed to reject every non-emptyContentsvalue, this file would still stay green. A single valid text case and a single valid media case would lock down the acceptance contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/embedding_multimodal_test.go` around lines 9 - 27, Add a happy-path validation test for both a text embedding and a media embedding: create a TestEmbeddingInputValidateAcceptsNonEmpty that constructs an EmbeddingInput with a non-empty Contents containing a single EmbeddingContentPart of Type EmbeddingContentPartTypeText with Text set and assert Validate() returns no error; and create a TestEmbeddingContentPartValidateAcceptsSingleMedia that constructs an EmbeddingContentPart of Type EmbeddingContentPartTypeImage with Image set to an EmbeddingMediaPart (use Ptr("https://example.com/img.png") or the existing helper) and assert part.Validate() returns nil; reference EmbeddingInput.Validate, EmbeddingContentPart.Validate, EmbeddingContentPartTypeImage/Text, and EmbeddingMediaPart to locate code.plugins/semanticcache/test_utils.go (1)
557-560: Usebifrost.Ptr()here instead of taking the address of a loop copy.This keeps the helper aligned with the repo convention and removes the extra temp variable.
♻️ Suggested cleanup
- for i, text := range texts { - t := text - contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: &t}} - } + for i, text := range texts { + contents[i] = schemas.EmbeddingContent{{ + Type: schemas.EmbeddingContentPartTypeText, + Text: bifrost.Ptr(text), + }} + }Based on learnings, prefer using
bifrost.Ptr()to create pointers instead of the address operator (&) even when&would be valid syntactically, and apply this consistently in test utilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/test_utils.go` around lines 557 - 560, The loop in the test helper creates a loop-local copy (t := text) and takes its address for EmbeddingContent.Text; replace that pattern with bifrost.Ptr(text) (i.e., set Text: bifrost.Ptr(text)) to follow the repo convention and remove the temporary variable; update the assignment in the loop that populates contents (and ensure the bifrost package is imported if not already) so schemas.EmbeddingContent uses bifrost.Ptr instead of &t.core/internal/llmtests/embedding.go (1)
61-65: Usebifrost.Ptr(...)for pointer creation in test payloads.This works, but for repo consistency prefer
bifrost.Ptr(text)over taking the address of a local variable.Suggested diff
contents := make([]schemas.EmbeddingContent, len(testTexts)) for i, text := range testTexts { - t := text - contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: &t}} + contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: bifrost.Ptr(text)}} }Based on learnings: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&), including test utilities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/internal/llmtests/embedding.go` around lines 61 - 65, Replace local-address pointer creation in the test payload by using bifrost.Ptr to match repo conventions: in the loop that builds contents (variable contents of type []schemas.EmbeddingContent) switch from creating a local t and taking &t to using bifrost.Ptr(text) for the Text field (the place where schemas.EmbeddingContent and schemas.EmbeddingContentPartTypeText are set). Ensure bifrost is imported if not already.core/providers/cohere/embedding_multimodal_test.go (1)
11-16: Use the pointer helper consistently in these fixtures.This file already uses
schemas.Ptr(...)for media fields, so using the same helper forTextavoids mixing two pointer-construction styles in the same test.♻️ Suggested cleanup
- text := "hello" req := ToCohereEmbeddingRequest(&schemas.BifrostEmbeddingRequest{ Model: "embed-v4.0", Input: &schemas.EmbeddingInput{ - Contents: []schemas.EmbeddingContent{{{Type: schemas.EmbeddingContentPartTypeText, Text: &text}}}, + Contents: []schemas.EmbeddingContent{{{Type: schemas.EmbeddingContentPartTypeText, Text: schemas.Ptr("hello")}}}, }, })- caption := "caption" req := ToCohereEmbeddingRequest(&schemas.BifrostEmbeddingRequest{ Model: "embed-v4.0", Input: &schemas.EmbeddingInput{ Contents: []schemas.EmbeddingContent{{ - {Type: schemas.EmbeddingContentPartTypeText, Text: &caption}, + {Type: schemas.EmbeddingContentPartTypeText, Text: schemas.Ptr("caption")}, {Type: schemas.EmbeddingContentPartTypeImage, Image: &schemas.EmbeddingMediaPart{URL: schemas.Ptr("https://example.com/cat.png")}}, }}, }, })Based on learnings, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically, including test utilities, using an equivalent helper when that is what is already in scope.
Also applies to: 25-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/cohere/embedding_multimodal_test.go` around lines 11 - 16, Replace direct address-of pointer usage for the EmbeddingInput.Text field with the existing pointer helper used elsewhere in the test: instead of &text use schemas.Ptr(text) (or bifrost.Ptr if that helper is in scope) when constructing the EmbeddingContent in ToCohereEmbeddingRequest/ schemas.BifrostEmbeddingRequest; update the other occurrences noted (around lines 25-30) so all pointer construction in this fixture is done via the same Ptr helper for consistency.core/providers/gemini/types.go (1)
1135-1137: Verify the new embedding flags use the same wire casing as the SDK.This file already normalizes snake_case Gemini payloads in several places, but
documentOcr/audioTrackExtractioncurrently only bind camelCase. If the SDK emitsdocument_ocr/audio_track_extraction, those options will be silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/types.go` around lines 1135 - 1137, The DocumentOCR and AudioTrackExtraction fields only accept camelCase JSON keys and will miss snake_case keys; update the struct that defines DocumentOCR and AudioTrackExtraction in core/providers/gemini/types.go to accept both wire casings by implementing a custom UnmarshalJSON for that struct: parse into a secondary map[string]json.RawMessage (or a small alias struct), check for both "documentOcr" and "document_ocr" (and "audioTrackExtraction" and "audio_track_extraction"), and set the DocumentOCR and AudioTrackExtraction boolean pointers accordingly, preserving existing behavior for other fields.core/providers/cohere/embedding.go (1)
117-127: Silentnilreturn on conversion error loses error context.When
cohereContentBlockFromEmbeddingPartfails at line 121 or 135, the function returnsnilwithout any indication of why the conversion failed. This makes debugging difficult.Consider logging the error or restructuring to return an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/cohere/embedding.go` around lines 117 - 127, The conversion loop silently returns nil on errors from cohereContentBlockFromEmbeddingPart, losing context; update the surrounding function (the routine building CohereEmbeddingInput and setting cohereReq.Inputs) to propagate errors instead of returning nil: change the function signature to return (..., error) if needed, capture the error from cohereContentBlockFromEmbeddingPart and return it (or wrap it with context) rather than returning nil, and ensure both occurrences (the loop at the first branch and the similar block at line ~135) follow the same error propagation so callers can log or handle the specific conversion error.core/providers/gemini/embedding.go (2)
140-163: Redundant nil checks and potential logic issue inapplyGeminiEmbeddingParams.Lines 152-154 and 158-160 delete from
req.ExtraParamsbut the preceding lines (151, 157) assign to the same map. The deletion is inside redundant nil checks after just assigning to the map. Additionally, the logic appears to extract a bool pointer from ExtraParams, then assign it back to ExtraParams, then delete it—which leaves the value unchanged.If the intent is to promote these fields to top-level request fields (like
DocumentOCRon the request struct), the current code doesn't achieve that.♻️ Suggested fix to clarify intent
If the goal is to extract these params and set top-level fields on the request, you'd need fields like
req.DocumentOCRandreq.AudioTrackExtraction. If those don't exist, consider removing this block or clarifying the intent:- if params.ExtraParams != nil { - if documentOCR, ok := schemas.SafeExtractBoolPointer(params.ExtraParams["documentOcr"]); ok { - req.ExtraParams["documentOcr"] = documentOCR - if req.ExtraParams != nil { - delete(req.ExtraParams, "documentOcr") - } - } - if audioTrackExtraction, ok := schemas.SafeExtractBoolPointer(params.ExtraParams["audioTrackExtraction"]); ok { - req.ExtraParams["audioTrackExtraction"] = audioTrackExtraction - if req.ExtraParams != nil { - delete(req.ExtraParams, "audioTrackExtraction") - } - } - } + // documentOcr and audioTrackExtraction are passed through ExtraParams + // and handled by Gemini's API directly - no special extraction needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/embedding.go` around lines 140 - 163, The function applyGeminiEmbeddingParams currently reads boolean values from params.ExtraParams with schemas.SafeExtractBoolPointer, assigns them back into req.ExtraParams and then immediately deletes them — the nil checks and deletes are redundant and the code does not promote these values to top-level fields; fix by either (A) removing this whole extraction/delete block if these flags should remain only in ExtraParams, or (B) if you intend to promote them, add top-level bool fields on the request (e.g., DocumentOCR and AudioTrackExtraction on GeminiEmbeddingRequest), set req.DocumentOCR and req.AudioTrackExtraction from the extracted pointers, and then delete those keys from req.ExtraParams; update applyGeminiEmbeddingParams to use params.ExtraParams -> SafeExtractBoolPointer -> set top-level req fields -> delete keys from req.ExtraParams (no redundant nil checks).
409-421: Silentnilreturn on conversion error obscures failure cause.When
geminiContentToEmbeddingContentreturns an error at lines 413 or 427, the function returnsnilwithout propagating the error. The caller cannot distinguish between a nil request input and a conversion failure.Consider either returning an error or logging the failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/embedding.go` around lines 409 - 421, The code silently returns nil when geminiContentToEmbeddingContent fails, losing error context; update the function that builds bifrostReq (the block handling request.Requests and uses geminiContentToEmbeddingContent, bifrostReq, and applyBifrostEmbeddingParams) to propagate the conversion error instead of returning nil—i.e., change the return path to return (nil, err) or wrap and return a descriptive error so callers can distinguish conversion failures, and if the function signature cannot change, at minimum log the error with context (including which req failed) before returning; ensure all callsites of this function are updated to handle the propagated error when you adjust the signature.core/providers/vertex/embedding.go (1)
96-109: Same redundant ExtraParams extraction pattern as Gemini provider.This block extracts boolean pointers from
params.ExtraParams, assigns them back toreq.ExtraParams(which is the same map), then deletes them. The net effect is no change. This mirrors the same issue incore/providers/gemini/embedding.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/embedding.go` around lines 96 - 109, The code redundantly reads boolean pointers from params.ExtraParams and writes them back to req.ExtraParams then deletes keys from the same map, causing no net effect; update the block around schemas.SafeExtractBoolPointer so you either (A) simply copy values from params.ExtraParams into req.ExtraParams without deleting, or (B) if the intent is to move keys, ensure you delete from params.ExtraParams (not req.ExtraParams) and only perform the delete when params.ExtraParams and req.ExtraParams are different maps; adjust the handling for the keys "documentOcr" and "audioTrackExtraction" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/internal/llmtests/account.go`:
- Around line 44-45: AllProviderConfigs' Vertex provider config is missing the
multimodal embedding flags so tests skip that scenario; update the Vertex entry
in AllProviderConfigs to set Scenarios.MultimodalEmbedding = true and set
MultimodalEmbeddingModel = "multimodalembedding@001" (and any corresponding
field assignment on the Vertex provider config struct) so the Scenarios and
model whitelist match the API key whitelist referenced earlier; locate the
Vertex provider config near the AllProviderConfigs variable and add those fields
to the config and its Scenarios struct assignment.
In `@core/internal/llmtests/embedding_multimodal.go`:
- Around line 47-86: validateNonEmptyVectors misses the case where a provider
returns a collapsed batch (one resp.Data entry with Embedding.Embedding2DArray);
update validateNonEmptyVectors to normalize that shape before iterating: if
len(resp.Data) == 1 and resp.Data[0].Embedding.Embedding2DArray != nil, treat
each row in Embedding2DArray as its own embedding entry (either by expanding
into a temporary slice of items or by iterating the 2D array directly), then use
getEmbeddingVector (or a small helper that extracts the vector from the 2D array
rows) to validate non-empty vectors and consistent dimensions for every expanded
embedding; also update the final t.Logf to report the expanded embedding count
instead of len(resp.Data).
In `@core/providers/bedrock/embedding.go`:
- Around line 11-13: The conversion function ToBedrockTitanEmbeddingRequest is
incorrectly collapsing the batch dimension by concatenating all Contents into a
single InputText; instead add a guard that asserts or returns an error if the
incoming request contains more than one content item (i.e., len(req.Contents) !=
1) so we only accept single-content requests here, and remove the logic that
stitches multiple Contents together (the code that builds a single InputText
from all parts); this ensures the core layer must fan out and prevents silently
returning one embedding for a multi-item request.
In `@core/providers/cohere/embedding.go`:
- Around line 195-200: The conversion currently silently drops extra images when
handling req.Images (case len(req.Images) > 0) and builds
bifrostReq.Input.Contents with only req.Images[0]; add explicit validation to
reject multiple images by checking if len(req.Images) > 1 and returning an error
(or propagate an appropriate conversion error) before constructing
bifrostReq.Input.Contents; reference the existing symbols req.Images,
bifrostReq.Input.Contents, EmbeddingContentPartTypeImage and EmbeddingMediaPart
when locating the change in core/providers/cohere/embedding.go.
In `@core/providers/gemini/embedding.go`:
- Around line 173-176: The current check conflates an empty contents slice with
an actual error and wraps a nil error (from EmbeddingInputToContents) causing
the %!w(<nil>) output; change the logic in embedding.go after calling
EmbeddingInputToContents to first handle err != nil and return fmt.Errorf("error
converting embedding input to contents: %w", err), and if err == nil but
len(contents) == 0 return a distinct non-wrapping error (e.g.,
fmt.Errorf("embedding input produced no contents")) so the message is clear when
contents is empty.
In `@core/providers/openai/embedding.go`:
- Around line 67-100: The converter currently flattens all parts across
bifrostReq.Input.Contents into global texts/tokenBatches, losing
per-EmbeddingContent boundaries; change the logic to iterate each content (each
entry in bifrostReq.Input.Contents) and build one OpenAIEmbeddingInput per
content: for each content, ensure parts are all the same type (error if mixed),
for text parts concatenate their strings into a single Text (e.g., join with a
space) and set OpenAIEmbeddingInput.Text, and for token parts concatenate token
slices into a single Embedding and set OpenAIEmbeddingInput.Embedding; finally
produce one input per content (a slice of OpenAIEmbeddingInput) instead of
flattening across contents so that each EmbeddingContent maps to one OpenAI
result (update code around bifrostReq.Input.Contents, OpenAIEmbeddingInput, and
the switch that currently sets Texts/Texts/Embeddings).
In `@core/providers/vertex/embedding.go`:
- Around line 149-152: Remove the stray debug print by deleting the
fmt.Println("b64", b64) call in core/providers/vertex/embedding.go; keep the
logic that calls extractBase64FromDataURI(*part.Image.Data) and assigns the
result to img.BytesBase64Encoded (variable b64), and ensure no other fmt.Println
debug statements remain in the same function or nearby code that handles
part.Image.Data or img.BytesBase64Encoded.
In `@core/providers/vertex/vertex.go`:
- Around line 1395-1402: The Embedding() method currently always fetches OAuth
credentials instead of supporting API-key auth like ChatStream() and
GenerateContent(); update Embedding() to first check key.Value.GetValue() and,
if non-empty, build authQuery and append it to the Gemini URL (use
getCompleteURLForGeminiEndpoint with ":embedContent" for
isGeminiEmbedding2Request) before making the request, otherwise fall back to the
existing OAuth flow (the same logic used in ChatStream()/GenerateContent());
ensure authQuery is used consistently when constructing the request URL so
VERTEX_API_KEY-only setups succeed.
In `@core/schemas/trace.go`:
- Line 219: The AttrDimensions constant was changed and that is a
telemetry-breaking change; revert AttrDimensions back to the existing legacy key
("gen_ai.embeddings.dimension.count") and introduce a new constant (e.g.,
AttrDimensionsV2 or AttrEmbeddingDimensionsKey) with the new wire key for
dual-writing during migration; update any writers to emit both constants where
needed but keep AttrDimensions unchanged to preserve
dashboards/filters/exporters that rely on it.
In `@plugins/logging/utils.go`:
- Around line 422-431: The loop building contentBlocks for logging currently
only appends parts where part.Type == schemas.EmbeddingContentPartTypeText,
dropping image/audio/video/file parts and producing empty user messages; update
the logic in the block that iterates request.EmbeddingRequest.Input.Contents to
also handle non-text parts by appending a schemas.ChatContentBlock for each
non-text part (e.g., set Type to a suitable value or to
schemas.ChatContentBlockTypeText with a Text placeholder) that records the
part.Type and any available metadata (filename, mime, size, description, or a
generated placeholder like "[image]" / "[audio]" / "[file]"). Ensure the new
branch references request.EmbeddingRequest.Input.Contents, contentBlocks,
part.Type, and schemas.ChatContentBlock so multimodal embedding inputs are
preserved in the log history.
In `@plugins/semanticcache/utils.go`:
- Around line 447-456: extractTextForEmbedding currently returns an empty string
for embedding requests that contain only non-text parts, which leads
generateEmbeddingsForStorage and performSemanticSearch to call
generateEmbedding("") and produce identical cache keys; update
extractTextForEmbedding to mirror Chat/Responses behavior by checking
len(textParts) > 0 and return a sentinel/error (or a boolean indicating "no
text") instead of an empty string, and update callers
(generateEmbeddingsForStorage and performSemanticSearch) to skip semantic
caching / embedding generation when that sentinel/no-text is returned so
multimodal-only embedding requests do not collide in the semantic cache.
In `@tests/integrations/python/tests/test_cohere.py`:
- Around line 159-196: Both tests request Cohere-only embedding features and
must skip when provider/model lacks those capabilities: update
test_06_custom_dimensions_embedding and test_07_multiple_embedding_types to
check supports_capability(provider, model, "<capability>") (from config_loader)
before running; for test_06 check capability for "embeddings_custom_dimensions"
or the equivalent capability used elsewhere (or guard specifically for
Cohere/embed-v4.0), and for test_07 check capability for "embeddings_int8" (or
guard for Cohere), calling pytest.skip(...) if unsupported — mirror the
capability-guard pattern used in test_pydanticai.py and use
get_provider_cohere_client/format_provider_model as before.
In `@tests/integrations/python/tests/test_google.py`:
- Around line 3091-3106: The test 'test_45' is inconsistent: the docstring says
"a text part" but the request builds contents with two identical text parts
using types.Content(parts=[types.Part(text=...), types.Part(text=...),
types.Part.from_bytes(...)]). Fix by either updating the docstring to say "two
text parts" or change the second types.Part(text=...) to a distinct text string
that reflects the intended test scenario so the intent and payload match.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 911-940: The empty-Contents guard in the embedding request path
rejects legacy callers because legacy fields (req.Input.Text, req.Input.Texts,
req.Input.Embedding, req.Input.Embeddings) are not normalized to
req.Input.Contents before the check; move or call the compatibility
normalization that populates req.Input.Contents for legacy fields before the if
that checks (req.Input == nil || len(req.Input.Contents) == 0) and
isLargePayloadPassthrough(ctx), or alternatively change that guard to also
accept legacy-field presence and run the normalization immediately prior to
calling req.Input.Validate(); ensure Validate() continues to run against the
normalized Contents so legacy callers remain supported.
---
Nitpick comments:
In `@core/internal/llmtests/embedding.go`:
- Around line 61-65: Replace local-address pointer creation in the test payload
by using bifrost.Ptr to match repo conventions: in the loop that builds contents
(variable contents of type []schemas.EmbeddingContent) switch from creating a
local t and taking &t to using bifrost.Ptr(text) for the Text field (the place
where schemas.EmbeddingContent and schemas.EmbeddingContentPartTypeText are
set). Ensure bifrost is imported if not already.
In `@core/providers/cohere/embedding_multimodal_test.go`:
- Around line 11-16: Replace direct address-of pointer usage for the
EmbeddingInput.Text field with the existing pointer helper used elsewhere in the
test: instead of &text use schemas.Ptr(text) (or bifrost.Ptr if that helper is
in scope) when constructing the EmbeddingContent in ToCohereEmbeddingRequest/
schemas.BifrostEmbeddingRequest; update the other occurrences noted (around
lines 25-30) so all pointer construction in this fixture is done via the same
Ptr helper for consistency.
In `@core/providers/cohere/embedding.go`:
- Around line 117-127: The conversion loop silently returns nil on errors from
cohereContentBlockFromEmbeddingPart, losing context; update the surrounding
function (the routine building CohereEmbeddingInput and setting
cohereReq.Inputs) to propagate errors instead of returning nil: change the
function signature to return (..., error) if needed, capture the error from
cohereContentBlockFromEmbeddingPart and return it (or wrap it with context)
rather than returning nil, and ensure both occurrences (the loop at the first
branch and the similar block at line ~135) follow the same error propagation so
callers can log or handle the specific conversion error.
In `@core/providers/gemini/embedding.go`:
- Around line 140-163: The function applyGeminiEmbeddingParams currently reads
boolean values from params.ExtraParams with schemas.SafeExtractBoolPointer,
assigns them back into req.ExtraParams and then immediately deletes them — the
nil checks and deletes are redundant and the code does not promote these values
to top-level fields; fix by either (A) removing this whole extraction/delete
block if these flags should remain only in ExtraParams, or (B) if you intend to
promote them, add top-level bool fields on the request (e.g., DocumentOCR and
AudioTrackExtraction on GeminiEmbeddingRequest), set req.DocumentOCR and
req.AudioTrackExtraction from the extracted pointers, and then delete those keys
from req.ExtraParams; update applyGeminiEmbeddingParams to use
params.ExtraParams -> SafeExtractBoolPointer -> set top-level req fields ->
delete keys from req.ExtraParams (no redundant nil checks).
- Around line 409-421: The code silently returns nil when
geminiContentToEmbeddingContent fails, losing error context; update the function
that builds bifrostReq (the block handling request.Requests and uses
geminiContentToEmbeddingContent, bifrostReq, and applyBifrostEmbeddingParams) to
propagate the conversion error instead of returning nil—i.e., change the return
path to return (nil, err) or wrap and return a descriptive error so callers can
distinguish conversion failures, and if the function signature cannot change, at
minimum log the error with context (including which req failed) before
returning; ensure all callsites of this function are updated to handle the
propagated error when you adjust the signature.
In `@core/providers/gemini/types.go`:
- Around line 1135-1137: The DocumentOCR and AudioTrackExtraction fields only
accept camelCase JSON keys and will miss snake_case keys; update the struct that
defines DocumentOCR and AudioTrackExtraction in core/providers/gemini/types.go
to accept both wire casings by implementing a custom UnmarshalJSON for that
struct: parse into a secondary map[string]json.RawMessage (or a small alias
struct), check for both "documentOcr" and "document_ocr" (and
"audioTrackExtraction" and "audio_track_extraction"), and set the DocumentOCR
and AudioTrackExtraction boolean pointers accordingly, preserving existing
behavior for other fields.
In `@core/providers/vertex/embedding.go`:
- Around line 96-109: The code redundantly reads boolean pointers from
params.ExtraParams and writes them back to req.ExtraParams then deletes keys
from the same map, causing no net effect; update the block around
schemas.SafeExtractBoolPointer so you either (A) simply copy values from
params.ExtraParams into req.ExtraParams without deleting, or (B) if the intent
is to move keys, ensure you delete from params.ExtraParams (not req.ExtraParams)
and only perform the delete when params.ExtraParams and req.ExtraParams are
different maps; adjust the handling for the keys "documentOcr" and
"audioTrackExtraction" accordingly.
In `@core/schemas/embedding_multimodal_test.go`:
- Around line 9-27: Add a happy-path validation test for both a text embedding
and a media embedding: create a TestEmbeddingInputValidateAcceptsNonEmpty that
constructs an EmbeddingInput with a non-empty Contents containing a single
EmbeddingContentPart of Type EmbeddingContentPartTypeText with Text set and
assert Validate() returns no error; and create a
TestEmbeddingContentPartValidateAcceptsSingleMedia that constructs an
EmbeddingContentPart of Type EmbeddingContentPartTypeImage with Image set to an
EmbeddingMediaPart (use Ptr("https://example.com/img.png") or the existing
helper) and assert part.Validate() returns nil; reference
EmbeddingInput.Validate, EmbeddingContentPart.Validate,
EmbeddingContentPartTypeImage/Text, and EmbeddingMediaPart to locate code.
In `@plugins/semanticcache/test_utils.go`:
- Around line 557-560: The loop in the test helper creates a loop-local copy (t
:= text) and takes its address for EmbeddingContent.Text; replace that pattern
with bifrost.Ptr(text) (i.e., set Text: bifrost.Ptr(text)) to follow the repo
convention and remove the temporary variable; update the assignment in the loop
that populates contents (and ensure the bifrost package is imported if not
already) so schemas.EmbeddingContent uses bifrost.Ptr instead of &t.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00ea3a71-28fe-40ce-befa-ae03e2960b6e
📒 Files selected for processing (37)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/embedding.gocore/internal/llmtests/embedding_multimodal.gocore/internal/llmtests/tests.gocore/providers/azure/azure.gocore/providers/bedrock/embedding.gocore/providers/cohere/cohere_test.gocore/providers/cohere/embedding.gocore/providers/cohere/embedding_multimodal_test.gocore/providers/cohere/types.gocore/providers/gemini/embedding.gocore/providers/gemini/embedding_multimodal_test.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/types.gocore/providers/huggingface/embedding.gocore/providers/openai/embedding.gocore/providers/openai/openai.gocore/providers/openai/types.gocore/providers/vertex/embedding.gocore/providers/vertex/embedding_multimodal_test.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vertex/vertex_test.gocore/schemas/embedding.gocore/schemas/embedding_multimodal_test.gocore/schemas/trace.goframework/tracing/llmspan.goplugins/logging/utils.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotests/integrations/python/config.ymltests/integrations/python/tests/test_cohere.pytests/integrations/python/tests/test_google.pytransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/cohere.go
d148be8 to
6f29fb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/internal/llmtests/response_validation.go (1)
1324-1348:⚠️ Potential issue | 🟠 MajorHandle Base64 embeddings in the dimension path too.
Line 1325 now treats
Base64as valid vector data, but Lines 1337-1348 and 1521-1531 never derive dimensions from it. A Base64-only embedding will pass the presence check and then failexpected_dimensionswith0, while metrics record the wrong dimension. Either decodeBase64here as well or exclude it fromhasDatauntil dimensions can be computed.As per coding guidelines, "
core/internal/llmtests/**/*.go: Scenario-based LLM tests incore/internal/llmtests/run against live provider APIs with dual-API testing."Also applies to: 1519-1533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/internal/llmtests/response_validation.go` around lines 1324 - 1348, The presence check currently accepts embedding.Embedding.Base64 as valid data but the dimensions-check logic (iterating response.Data and computing actualDimensions from e.Float, e.Int8, e.Uint8, e.Binary, e.Ubinary) never handles Base64, causing Base64-only embeddings to report dimension 0 and fail expected_dimensions; update the dimension path to detect e.Base64, decode the base64 payload to raw bytes and set actualDimensions = len(decodedBytes) (using standard base64 decode), and update any other dimension/metrics computation that reads actualDimensions (the loop over response.Data and the metrics block that computes embedding dimensions) to use the decoded length so Base64-only embeddings are validated correctly; alternatively, if you prefer not to decode here, remove Base64 from the initial hasData check in the embedding presence check (embedding.Embedding.Base64) so Base64-only embeddings are not treated as having data.ui/app/workspace/logs/sheets/logDetailView.tsx (1)
952-953:⚠️ Potential issue | 🟠 MajorUse the same embedding object discriminator as the render path.
This helper checks
log.object === "list", but the new embedding UI path at Line 766 treats embedding logs as"embedding". As written, Copy request body never enters the embedding branch for the logs this component now renders, and falls back to the unsupported-type toast instead.Suggested fix
- const isEmbedding = log.object === "list"; + const isEmbedding = log.object === "embedding";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/sheets/logDetailView.tsx` around lines 952 - 953, The isEmbedding discriminator currently checks log.object === "list" but the render path uses "embedding", so update the condition in logDetailView.tsx (the isEmbedding variable) to match the render path (e.g., check log.object === "embedding" or accept both "embedding" and "list" if backward compatibility is needed) so the "Copy request body" flow enters the embedding branch; locate and update the isEmbedding declaration and any dependent branches that gate the embedding copy behavior to use the corrected discriminator.core/providers/vertex/vertex.go (1)
1520-1548:⚠️ Potential issue | 🟡 MinorPreserve
RawRequestcapture in the new embedding response path.These branches bypass
providerUtils.HandleProviderResponse, sobifrostResponse.ExtraFields.RawRequestis never set for Vertex embeddings anymore whensendBackRawRequestis enabled. That regresses raw request logging/debugging for this operation; please keep the typed unmarshal, but restore the common raw request/response capture path before converting to the Bifrost response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/vertex.go` around lines 1520 - 1548, The Vertex embedding branch skips the common raw request/response capture so bifrostResponse.ExtraFields.RawRequest isn't populated when provider.sendBackRawRequest is enabled; restore that capture before converting to BifrostEmbeddingResponse by invoking the same handling used elsewhere (e.g., call providerUtils.HandleProviderResponse or otherwise unmarshal jsonBody into a map and set bifrostResponse.ExtraFields.RawRequest) in both branches (the gemini.GeminiEmbeddingResponse path and the VertexEmbeddingResponse path) using the existing flags provider.sendBackRawRequest/provider.sendBackRawResponse and functions providerUtils.ShouldSendBackRawResponse and providerUtils.EnrichError to keep error handling consistent. Ensure the RawRequest is set prior to assigning bifrostResponse.Model/ExtraFields so the typed unmarshalling (VertexEmbeddingResponse.ToBifrostEmbeddingResponse / gemini.ToBifrostEmbeddingResponse) remains unchanged.core/providers/gemini/types.go (1)
1255-1279:⚠️ Potential issue | 🟠 MajorHandle nested snake_case fields inside
file_data.This only aliases the outer
file_datakey.FileDatastill has camelCase-only tags, so payloads like{"file_data":{"file_uri":"...","mime_type":"..."}}will unmarshal to a non-nilFileDatawith empty fields. That makes the new snake_case path fail downstream even though the JSON shape is otherwise valid.💡 Suggested follow-up
func (f *FileData) UnmarshalJSON(data []byte) error { type alias struct { DisplayName string `json:"displayName,omitempty"` DisplayNameSnake string `json:"display_name,omitempty"` FileURI string `json:"fileUri,omitempty"` FileURISnake string `json:"file_uri,omitempty"` MIMEType string `json:"mimeType,omitempty"` MIMETypeSnake string `json:"mime_type,omitempty"` } var aux alias if err := sonic.Unmarshal(data, &aux); err != nil { return err } f.DisplayName = aux.DisplayName if f.DisplayName == "" { f.DisplayName = aux.DisplayNameSnake } f.FileURI = aux.FileURI if f.FileURI == "" { f.FileURI = aux.FileURISnake } f.MIMEType = aux.MIMEType if f.MIMEType == "" { f.MIMEType = aux.MIMETypeSnake } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/types.go` around lines 1255 - 1279, The outer aliasing in PartAlias doesn't handle snake_case keys nested inside file_data because FileData's struct tags are camelCase-only; implement a custom UnmarshalJSON on the FileData type that defines an internal alias with both camelCase and snake_case json tags (e.g., DisplayName/display_name, FileURI/file_uri, MIMEType/mime_type), unmarshal into that aux, and then populate FileData's fields by preferring the camelCase values and falling back to the snake_case values when empty so that p.FileData (and any code using FileData) receives the actual nested values.
♻️ Duplicate comments (4)
core/providers/cohere/embedding.go (1)
195-200:⚠️ Potential issue | 🟠 MajorReject
images[]requests with more than one image.This still converts only
req.Images[0], so additional images are silently dropped instead of being rejected or documented as unsupported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/cohere/embedding.go` around lines 195 - 200, The code in embedding.go silently drops additional images by only converting req.Images[0]; update the handler that checks req.Images (the case starting with "case len(req.Images) > 0") to explicitly reject requests where len(req.Images) > 1 by returning an error (or appropriate response) instead of proceeding, and keep the existing behavior for exactly one image by populating bifrostReq.Input.Contents with a single schemas.EmbeddingContent entry using schemas.EmbeddingContentPartTypeImage and schemas.EmbeddingMediaPart{URL: &url}; ensure the error message clearly states that multiple images are unsupported and references req.Images so callers understand the validation.plugins/semanticcache/utils.go (1)
450-459:⚠️ Potential issue | 🟠 MajorSkip semantic caching when an embedding request has no text parts.
This branch still returns
""with no error for image/audio/file-onlyEmbeddingInput.Contents. The semantic-cache lookup/store paths then embed the empty string, so distinct multimodal-only embedding requests collapse onto the same semantic-cache vector. Please mirror the chat/responses branches here by failing fast (or returning a sentinel) and letting callers bypass semantic caching for those requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/utils.go` around lines 450 - 459, After collecting textParts from req.EmbeddingRequest.Input.Contents (the loop that checks part.Type == schemas.EmbeddingContentPartTypeText), detect when len(textParts) == 0 and fail fast instead of returning an empty string: return a sentinel error (e.g., ErrNoTextParts) or the existing "skip cache" sentinel used by chat/responses, along with metadataHash, so callers can bypass semantic caching; if ErrNoTextParts doesn't exist, add it as a package-level var and use it here.tests/integrations/python/tests/test_cohere.py (1)
159-196:⚠️ Potential issue | 🟠 MajorGate these Cohere-only embedding features behind provider capabilities.
These two tests still run against every provider in the generic
embeddingsscenario, butoutput_dimension=512andembedding_types=["float", "int8"]are feature-specific knobs. Without a capability check, the suite fails on unsupported providers with expected API errors instead of catching transport regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrations/python/tests/test_cohere.py` around lines 159 - 196, Both tests (test_06_custom_dimensions_embedding and test_07_multiple_embedding_types) run for all providers but rely on Cohere-only features; add capability checks and skip when the current provider/model doesn't support them. Use the existing test helpers (e.g., get_provider_cohere_client, format_provider_model, get_cross_provider_params_for_scenario) to query provider capabilities and gate: in test_06 check for a capability like support for output_dimension/custom_dimensions (or a provider capability flag for 512-d embeddings) and call pytest.skip if missing; in test_07 check for support of multiple embedding types / int8 embeddings and skip if not present. Ensure the checks run before creating the client so unsupported providers are skipped rather than failing.core/providers/bedrock/embedding.go (1)
12-40:⚠️ Potential issue | 🔴 CriticalDon’t flatten multiple
Contentsentries into one Titan request.
Contentsis the batch dimension in this PR. Concatenating every content entry into oneInputTextsilently turns an N-item request into a single Titan embedding, so the response shape no longer matches the caller’s input. This converter should accept exactly one content item and only aggregate parts within that item.🔧 Safer shape guard
-// Titan produces a single embedding per request. All text parts across all content entries -// are stitched together into one input string. Non-text parts are not supported. +// Titan produces a single embedding per request. Batching across content entries +// must be handled upstream; only parts within one content item are aggregated. func ToBedrockTitanEmbeddingRequest(bifrostReq *schemas.BifrostEmbeddingRequest) (*BedrockTitanEmbeddingRequest, error) { if bifrostReq == nil { return nil, fmt.Errorf("bifrost embedding request is nil") } if bifrostReq.Input == nil || len(bifrostReq.Input.Contents) == 0 { return nil, fmt.Errorf("no input provided for Titan embedding") } + if len(bifrostReq.Input.Contents) != 1 { + return nil, fmt.Errorf("amazon titan embedding models support exactly one content item per request") + } if bifrostReq.Params != nil && bifrostReq.Params.Dimensions != nil { return nil, fmt.Errorf("amazon Titan embedding models do not support custom dimensions parameter") } var sb strings.Builder - for _, content := range bifrostReq.Input.Contents { - for _, part := range content { - if part.Type != schemas.EmbeddingContentPartTypeText || part.Text == nil { - return nil, fmt.Errorf("amazon Titan embedding models only support text input") - } - sb.WriteString(*part.Text) - sb.WriteString(" \n") - } + for _, part := range bifrostReq.Input.Contents[0] { + if part.Type != schemas.EmbeddingContentPartTypeText || part.Text == nil { + return nil, fmt.Errorf("amazon Titan embedding models only support text input") + } + sb.WriteString(*part.Text) + sb.WriteString(" \n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/embedding.go` around lines 12 - 40, The converter ToBedrockTitanEmbeddingRequest currently concatenates all entries in bifrostReq.Input.Contents into one BedrockTitanEmbeddingRequest.InputText, collapsing the batch dimension; change it to require exactly one content item (error if len(bifrostReq.Input.Contents) != 1) and only iterate over that single content's parts to build InputText, preserving the 1:1 request/response shape; keep the existing validation for part types (schemas.EmbeddingContentPartTypeText) and the BedrockTitanEmbeddingRequest construction unchanged.
🧹 Nitpick comments (5)
plugins/semanticcache/test_utils.go (1)
557-559: Preferbifrost.Ptrover taking&there.This repo standardizes on the pointer helper in test utilities, and it lets you drop the extra temporary.
♻️ Suggested cleanup
Input: func() *schemas.EmbeddingInput { contents := make([]schemas.EmbeddingContent, len(texts)) for i, text := range texts { - t := text - contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: &t}} + contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: bifrost.Ptr(text)}} } return &schemas.EmbeddingInput{Contents: contents} }(),Based on learnings, "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/test_utils.go` around lines 557 - 559, Replace the direct address operator usage (&t) when populating schemas.EmbeddingContent in the loop over texts with the repo-standard pointer helper bifrost.Ptr(t); specifically update the assignment that currently sets Text: &t to Text: bifrost.Ptr(t) and add an import for the bifrost package if it's not already present so the test_utils.go loop that builds contents uses bifrost.Ptr consistently.core/providers/fireworks/fireworks_test.go (1)
191-193: Usebifrost.Ptrfor the text part pointer.This test now takes the address of a temporary just to build
*string. Prefer the repo pointer helper here for consistency.♻️ Suggested cleanup
Input: &schemas.EmbeddingInput{ Contents: []schemas.EmbeddingContent{ - {{Type: schemas.EmbeddingContentPartTypeText, Text: &text}}, + {{Type: schemas.EmbeddingContentPartTypeText, Text: bifrost.Ptr(text)}}, }, },Based on learnings, "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/fireworks/fireworks_test.go` around lines 191 - 193, The test currently takes the address of a temporary string (variable text) when constructing the Contents slice of schemas.EmbeddingContent; replace that pattern by using the repository pointer helper bifrost.Ptr to create the *string instead. Update the EmbeddingContent construction (the Contents: []schemas.EmbeddingContent{...} entry where Type is schemas.EmbeddingContentPartTypeText) to call bifrost.Ptr(yourText) rather than &text so the pointer creation is consistent with other tests and utilities.core/providers/gemini/embedding.go (2)
386-394: Error context lost on conversion failure.When
geminiContentToEmbeddingContentfails, the method returnsnilwithout any indication of the error. This makes debugging difficult. The same pattern at lines 400-403.Consider logging the error or changing the method signature to return an error if this aligns with other converter patterns in the stack.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/embedding.go` around lines 386 - 394, The conversion loop using geminiContentToEmbeddingContent silently swallows errors by returning nil; update the converter to propagate or log conversion failures: modify the surrounding function that builds bifrostReq (reference bifrostReq, geminiContentToEmbeddingContent and applyBifrostEmbeddingParams) to return an error (or at minimum log the error via the existing logger) when geminiContentToEmbeddingContent fails, and ensure callers handle the returned error instead of getting a nil result; apply the same change to the analogous block at lines 400-403 so errors are not lost.
153-155: Error message could be clearer when contents array is empty.The error "bifrost request is nil or input is nil" also triggers when
len(bifrostReq.Input.Contents) == 0, which may confuse debugging since Input is technically not nil in that case.✏️ Suggested improvement
if bifrostReq == nil || bifrostReq.Input == nil || len(bifrostReq.Input.Contents) == 0 { - return nil, fmt.Errorf("bifrost request is nil or input is nil") + return nil, fmt.Errorf("bifrost request, input, or contents is nil/empty") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/embedding.go` around lines 153 - 155, The current nil-check lumps three failure modes into one message; update the validation around bifrostReq (the block testing bifrostReq, bifrostReq.Input, and len(bifrostReq.Input.Contents)) to return distinct, clear errors for each case (e.g., "bifrost request is nil", "bifrost request input is nil", and "bifrost request input contents is empty") so callers can tell whether Input exists but Contents is empty; adjust the fmt.Errorf calls accordingly where bifrostReq and bifrostReq.Input.Contents are validated.core/providers/vertex/embedding.go (1)
40-46: Silent failure on non-text content parts may confuse users.When a non-text part is encountered, the function silently returns
nil. Since the doc comment states "All contents must be text-only," a logged warning or a clearer signal would help users understand why their multimodal input failed on the text embedding endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/embedding.go` around lines 40 - 46, The loop that builds sb silently returns nil when encountering a non-text part (checking part.Type != schemas.EmbeddingContentPartTypeText || part.Text == nil), which hides why embedding failed; update this in the function that assembles content (the loop using sb and iterating over content/part) to log a warning including the offending part.Type (or part index) and return an explicit error or clear failure value instead of nil so callers get a clear signal that non-text content was rejected; use the package logger or existing request logger to emit the warning and replace the silent return with a descriptive error return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/internal/llmtests/utils.go`:
- Around line 627-669: getEmbeddingVector currently ignores bit-packed
embeddings (Binary / Ubinary) and returns "no valid embedding data found";
update getEmbeddingVector to detect and handle e.Binary and e.Ubinary similarly
to the base64 path: if either is present, treat the byte slice as little-endian
float32-packed data, verify length%4==0, iterate 4-byte chunks with
binary.LittleEndian.Uint32 and math.Float32frombits to populate and return a
[]float64, and include the same error handling as the base64 branch for bad
lengths; reference the function name getEmbeddingVector and the embedding fields
e.Binary and e.Ubinary when making the change.
In `@core/providers/cohere/embedding.go`:
- Around line 90-95: The converter is aliasing bifrostReq.Params.ExtraParams
into cohereReq.ExtraParams and then mutating it, causing side effects; instead,
when Params.ExtraParams is non-nil create a new map, copy each key/value into it
(e.g., make(map[string]interface{}, len(...)) and loop to assign), set
cohereReq.ExtraParams to that clone, and only mutate the clone; apply the same
cloning approach wherever the converters copy ExtraParams (the other converter
blocks referenced around the later ranges) so conversion functions remain pure
and do not modify the caller's map.
- Around line 334-367: cohereResp.Embeddings is never initialized and
hasFloat/hasBase64 are never set, causing a panic on the first append and
incorrect final assignment; fix by initializing cohereResp.Embeddings (or
embeddingData) before any appends, set hasFloat = true when encountering
emb.Float and hasBase64 = true when encountering emb.Base64 while iterating
bifrostResp.Data, append into the initialized embedding struct (e.g.,
embeddingData.Float, embeddingData.Base64, etc.), and after the loop assign
cohereResp.Embeddings = embeddingData only if hasFloat || hasBase64 (otherwise
leave nil or set appropriate type marker). Ensure you reference
CohereEmbeddingResponse, embeddingData, hasFloat, hasBase64 and the loop over
bifrostResp.Data when making the change.
In `@core/providers/gemini/embedding.go`:
- Around line 136-146: The code mutates the caller's params.ExtraParams map by
assigning it to req.ExtraParams and then deleting keys; fix this in the
embedding conversion by making a shallow copy of params.ExtraParams (handle nil)
and assign that copy to req.ExtraParams, then run schemas.SafeExtractBoolPointer
on the copied map for "documentOcr" and "audioTrackExtraction" and delete those
keys from the copy instead of the original; keep using the same helper
schemas.SafeExtractBoolPointer and set req.DocumentOCR and
req.AudioTrackExtraction accordingly.
In `@core/providers/huggingface/embedding.go`:
- Around line 98-111: The extractTextFromContent function currently concatenates
text parts without separators, collapsing boundaries (e.g., ["foo","bar"] ->
"foobar"); update extractTextFromContent to insert a delimiter (e.g., a single
space or newline) between parts when appending to the strings.Builder so each
part boundary is preserved while keeping the existing validation (check
part.Type == schemas.EmbeddingContentPartTypeText and part.Text != nil) and the
empty-content error; modify the loop in extractTextFromContent to write the
delimiter only when sb.Len() > 0 before appending the next *part.Text.
In `@core/providers/openai/types.go`:
- Around line 53-58: The OpenAIEmbeddingInput flattening causes multiple input
parts from a single schemas.EmbeddingContent to be sent as separate upstream
inputs; update ToOpenAIEmbeddingRequest to preserve one EmbeddingContent per
OpenAI input by aggregating parts of each schemas.EmbeddingContent before
populating OpenAIEmbeddingInput.Texts and .Embeddings (and any tokenBatches
logic) so each element of request.Input.Contents maps to exactly one
OpenAIEmbeddingInput entry; locate and change the logic in
ToOpenAIEmbeddingRequest (core/providers/openai/embedding.go) that iterates
request.Input.Contents and currently appends parts into single slices to instead
combine/concat parts per content and push one OpenAIEmbeddingInput per content,
ensuring fields Text, Texts, Embedding, or Embeddings are set appropriately for
that aggregated content.
In `@core/providers/vertex/embedding.go`:
- Around line 95-105: The code mutates the input map by assigning
params.ExtraParams to req.ExtraParams and then deleting keys; change this to
create a shallow copy of params.ExtraParams and assign the copy to
req.ExtraParams, then perform the SafeExtractBoolPointer checks (for
"documentOcr" and "audioTrackExtraction") and delete those keys from the copied
map while setting req.DocumentOCR and req.AudioTrackExtraction; reference the
symbols params.ExtraParams, req.ExtraParams, schemas.SafeExtractBoolPointer,
req.DocumentOCR, and req.AudioTrackExtraction to locate and update the logic so
the original params.ExtraParams remains unmodified.
In `@framework/logstore/migrations.go`:
- Around line 2413-2453: The migration only reads input_history[0], dropping
subsequent legacy inputs; update the SQL in migrations.go (the UPDATE that sets
embedding_input for object_type='embedding') to iterate over all entries in
input_history instead of hard-coding the first element: for Postgres, replace
(input_history::jsonb -> 0 -> 'content' -> 'content_blocks') with a full
expansion like jsonb_array_elements(input_history::jsonb) -> 'content' ->
'content_blocks' (or nested jsonb_array_elements twice) so you aggregate text
blocks from every history item before jsonb_agg into embedding_input; for
SQLite, change json_extract(input_history, '$[0].content.content_blocks') and
its json_each to iterate over all array entries (e.g., json_each(input_history)
then json_each(json_extract(..., '$.content.content_blocks')) ) so
json_group_array collects blocks from every history entry while keeping the
existing filters and embedding_input/null checks. Ensure the aggregate still
builds objects with type='text' and non-empty text fields.
In `@plugins/logging/main.go`:
- Around line 207-208: The EmbeddingInput field currently stores full
schemas.EmbeddingContent (including Image/Audio/File/Video.Data) which can
contain large/base64 or sensitive blobs; before assigning to EmbeddingInput, run
the same truncation/redaction utility used for transcription and image inputs
(the threshold-based redactor used elsewhere) to strip or truncate raw Data
fields and large payloads, and assign the sanitized result to EmbeddingInput;
update all assignment sites including the other occurrences noted around the
block at lines 480-483 to use the redaction helper so logs never receive full
binary/base64 content.
In `@tests/integrations/python/tests/test_cohere.py`:
- Line 50: The parameter annotation for assert_valid_cohere_embedding_response
uses an implicit optional (expected_dimensions: int = None); change it to an
explicit optional type (expected_dimensions: int | None = None or
expected_dimensions: Optional[int] = None) and, if using Optional, add the
necessary typing import; update the function signature and any related type
hints accordingly.
In `@ui/app/workspace/logs/sheets/logDetailView.tsx`:
- Around line 1016-1022: The current branch (inside the isEmbedding case)
flattens log.embedding_input into a texts array and drops non-text parts;
instead, preserve full EmbeddingContent semantics by assigning the original
embedding_input structure to requestBody.input when log.embedding_input exists
(i.e., requestBody.input = log.embedding_input) so each EmbeddingContent maps to
one output embedding; keep the existing text-only extraction and fallback only
when embedding_input is absent or empty (use the map/filter/flat logic only for
older logs that lack embedding_input) and ensure any type expectations for
requestBody.input are respected when switching to the full embedding_input
structure.
---
Outside diff comments:
In `@core/internal/llmtests/response_validation.go`:
- Around line 1324-1348: The presence check currently accepts
embedding.Embedding.Base64 as valid data but the dimensions-check logic
(iterating response.Data and computing actualDimensions from e.Float, e.Int8,
e.Uint8, e.Binary, e.Ubinary) never handles Base64, causing Base64-only
embeddings to report dimension 0 and fail expected_dimensions; update the
dimension path to detect e.Base64, decode the base64 payload to raw bytes and
set actualDimensions = len(decodedBytes) (using standard base64 decode), and
update any other dimension/metrics computation that reads actualDimensions (the
loop over response.Data and the metrics block that computes embedding
dimensions) to use the decoded length so Base64-only embeddings are validated
correctly; alternatively, if you prefer not to decode here, remove Base64 from
the initial hasData check in the embedding presence check
(embedding.Embedding.Base64) so Base64-only embeddings are not treated as having
data.
In `@core/providers/gemini/types.go`:
- Around line 1255-1279: The outer aliasing in PartAlias doesn't handle
snake_case keys nested inside file_data because FileData's struct tags are
camelCase-only; implement a custom UnmarshalJSON on the FileData type that
defines an internal alias with both camelCase and snake_case json tags (e.g.,
DisplayName/display_name, FileURI/file_uri, MIMEType/mime_type), unmarshal into
that aux, and then populate FileData's fields by preferring the camelCase values
and falling back to the snake_case values when empty so that p.FileData (and any
code using FileData) receives the actual nested values.
In `@core/providers/vertex/vertex.go`:
- Around line 1520-1548: The Vertex embedding branch skips the common raw
request/response capture so bifrostResponse.ExtraFields.RawRequest isn't
populated when provider.sendBackRawRequest is enabled; restore that capture
before converting to BifrostEmbeddingResponse by invoking the same handling used
elsewhere (e.g., call providerUtils.HandleProviderResponse or otherwise
unmarshal jsonBody into a map and set bifrostResponse.ExtraFields.RawRequest) in
both branches (the gemini.GeminiEmbeddingResponse path and the
VertexEmbeddingResponse path) using the existing flags
provider.sendBackRawRequest/provider.sendBackRawResponse and functions
providerUtils.ShouldSendBackRawResponse and providerUtils.EnrichError to keep
error handling consistent. Ensure the RawRequest is set prior to assigning
bifrostResponse.Model/ExtraFields so the typed unmarshalling
(VertexEmbeddingResponse.ToBifrostEmbeddingResponse /
gemini.ToBifrostEmbeddingResponse) remains unchanged.
In `@ui/app/workspace/logs/sheets/logDetailView.tsx`:
- Around line 952-953: The isEmbedding discriminator currently checks log.object
=== "list" but the render path uses "embedding", so update the condition in
logDetailView.tsx (the isEmbedding variable) to match the render path (e.g.,
check log.object === "embedding" or accept both "embedding" and "list" if
backward compatibility is needed) so the "Copy request body" flow enters the
embedding branch; locate and update the isEmbedding declaration and any
dependent branches that gate the embedding copy behavior to use the corrected
discriminator.
---
Duplicate comments:
In `@core/providers/bedrock/embedding.go`:
- Around line 12-40: The converter ToBedrockTitanEmbeddingRequest currently
concatenates all entries in bifrostReq.Input.Contents into one
BedrockTitanEmbeddingRequest.InputText, collapsing the batch dimension; change
it to require exactly one content item (error if len(bifrostReq.Input.Contents)
!= 1) and only iterate over that single content's parts to build InputText,
preserving the 1:1 request/response shape; keep the existing validation for part
types (schemas.EmbeddingContentPartTypeText) and the
BedrockTitanEmbeddingRequest construction unchanged.
In `@core/providers/cohere/embedding.go`:
- Around line 195-200: The code in embedding.go silently drops additional images
by only converting req.Images[0]; update the handler that checks req.Images (the
case starting with "case len(req.Images) > 0") to explicitly reject requests
where len(req.Images) > 1 by returning an error (or appropriate response)
instead of proceeding, and keep the existing behavior for exactly one image by
populating bifrostReq.Input.Contents with a single schemas.EmbeddingContent
entry using schemas.EmbeddingContentPartTypeImage and
schemas.EmbeddingMediaPart{URL: &url}; ensure the error message clearly states
that multiple images are unsupported and references req.Images so callers
understand the validation.
In `@plugins/semanticcache/utils.go`:
- Around line 450-459: After collecting textParts from
req.EmbeddingRequest.Input.Contents (the loop that checks part.Type ==
schemas.EmbeddingContentPartTypeText), detect when len(textParts) == 0 and fail
fast instead of returning an empty string: return a sentinel error (e.g.,
ErrNoTextParts) or the existing "skip cache" sentinel used by chat/responses,
along with metadataHash, so callers can bypass semantic caching; if
ErrNoTextParts doesn't exist, add it as a package-level var and use it here.
In `@tests/integrations/python/tests/test_cohere.py`:
- Around line 159-196: Both tests (test_06_custom_dimensions_embedding and
test_07_multiple_embedding_types) run for all providers but rely on Cohere-only
features; add capability checks and skip when the current provider/model doesn't
support them. Use the existing test helpers (e.g., get_provider_cohere_client,
format_provider_model, get_cross_provider_params_for_scenario) to query provider
capabilities and gate: in test_06 check for a capability like support for
output_dimension/custom_dimensions (or a provider capability flag for 512-d
embeddings) and call pytest.skip if missing; in test_07 check for support of
multiple embedding types / int8 embeddings and skip if not present. Ensure the
checks run before creating the client so unsupported providers are skipped
rather than failing.
---
Nitpick comments:
In `@core/providers/fireworks/fireworks_test.go`:
- Around line 191-193: The test currently takes the address of a temporary
string (variable text) when constructing the Contents slice of
schemas.EmbeddingContent; replace that pattern by using the repository pointer
helper bifrost.Ptr to create the *string instead. Update the EmbeddingContent
construction (the Contents: []schemas.EmbeddingContent{...} entry where Type is
schemas.EmbeddingContentPartTypeText) to call bifrost.Ptr(yourText) rather than
&text so the pointer creation is consistent with other tests and utilities.
In `@core/providers/gemini/embedding.go`:
- Around line 386-394: The conversion loop using geminiContentToEmbeddingContent
silently swallows errors by returning nil; update the converter to propagate or
log conversion failures: modify the surrounding function that builds bifrostReq
(reference bifrostReq, geminiContentToEmbeddingContent and
applyBifrostEmbeddingParams) to return an error (or at minimum log the error via
the existing logger) when geminiContentToEmbeddingContent fails, and ensure
callers handle the returned error instead of getting a nil result; apply the
same change to the analogous block at lines 400-403 so errors are not lost.
- Around line 153-155: The current nil-check lumps three failure modes into one
message; update the validation around bifrostReq (the block testing bifrostReq,
bifrostReq.Input, and len(bifrostReq.Input.Contents)) to return distinct, clear
errors for each case (e.g., "bifrost request is nil", "bifrost request input is
nil", and "bifrost request input contents is empty") so callers can tell whether
Input exists but Contents is empty; adjust the fmt.Errorf calls accordingly
where bifrostReq and bifrostReq.Input.Contents are validated.
In `@core/providers/vertex/embedding.go`:
- Around line 40-46: The loop that builds sb silently returns nil when
encountering a non-text part (checking part.Type !=
schemas.EmbeddingContentPartTypeText || part.Text == nil), which hides why
embedding failed; update this in the function that assembles content (the loop
using sb and iterating over content/part) to log a warning including the
offending part.Type (or part index) and return an explicit error or clear
failure value instead of nil so callers get a clear signal that non-text content
was rejected; use the package logger or existing request logger to emit the
warning and replace the silent return with a descriptive error return.
In `@plugins/semanticcache/test_utils.go`:
- Around line 557-559: Replace the direct address operator usage (&t) when
populating schemas.EmbeddingContent in the loop over texts with the
repo-standard pointer helper bifrost.Ptr(t); specifically update the assignment
that currently sets Text: &t to Text: bifrost.Ptr(t) and add an import for the
bifrost package if it's not already present so the test_utils.go loop that
builds contents uses bifrost.Ptr consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51b97aa5-0857-43ae-ad75-271ffc131093
📒 Files selected for processing (50)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/embedding.gocore/internal/llmtests/embedding_multimodal.gocore/internal/llmtests/response_validation.gocore/internal/llmtests/tests.gocore/internal/llmtests/utils.gocore/providers/azure/azure.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/cohere/cohere_test.gocore/providers/cohere/embedding.gocore/providers/cohere/embedding_multimodal_test.gocore/providers/cohere/types.gocore/providers/fireworks/fireworks_test.gocore/providers/gemini/embedding.gocore/providers/gemini/embedding_multimodal_test.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/types.gocore/providers/huggingface/embedding.gocore/providers/huggingface/huggingface_test.gocore/providers/openai/embedding.gocore/providers/openai/openai.gocore/providers/openai/types.gocore/providers/vertex/embedding.gocore/providers/vertex/embedding_multimodal_test.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vertex/vertex_test.gocore/schemas/embedding.gocore/schemas/embedding_multimodal_test.gocore/schemas/serialization_test.gocore/schemas/trace.goframework/logstore/migrations.goframework/logstore/tables.goframework/tracing/llmspan.goplugins/logging/main.goplugins/logging/utils.goplugins/logging/writer.goplugins/semanticcache/go.modplugins/semanticcache/plugin_core_test.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotests/integrations/python/config.ymltests/integrations/python/tests/test_cohere.pytests/integrations/python/tests/test_google.pytransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/cohere.goui/app/workspace/logs/sheets/logDetailView.tsx
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_core_test.go
✅ Files skipped from review due to trivial changes (6)
- core/schemas/embedding_multimodal_test.go
- core/providers/openai/openai.go
- core/providers/vertex/embedding_multimodal_test.go
- core/providers/gemini/embedding_multimodal_test.go
- core/providers/cohere/embedding_multimodal_test.go
- core/providers/openai/embedding.go
🚧 Files skipped from review as they are similar to previous changes (14)
- core/providers/cohere/cohere_test.go
- core/providers/cohere/types.go
- core/internal/llmtests/embedding.go
- core/providers/gemini/gemini.go
- core/providers/vertex/vertex_test.go
- core/schemas/trace.go
- core/providers/gemini/gemini_test.go
- transports/bifrost-http/handlers/inference.go
- tests/integrations/python/config.yml
- core/internal/llmtests/account.go
- tests/integrations/python/tests/test_google.py
- core/schemas/embedding.go
- framework/tracing/llmspan.go
- core/providers/vertex/types.go
6f29fb7 to
4f21ee8
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/providers/bedrock/invoke.go (1)
1002-1019:⚠️ Potential issue | 🟠 MajorPreserve non-float embedding types in the invoke response shim.
This path always reads
d.Embedding.Floatand always emitsresponse_type: "embeddings_floats", but the request path still acceptsembedding_types. If the provider returnsInt8,Uint8,Binary,Ubinary, orBase64, the compat response will come back empty or mislabeled instead of matching the requested type.At minimum, branch on the populated
EmbeddingsByTypefield and emit the matching native envelope, or fail loudly instead of silently flattening everything to float.Based on learnings: In maximhq/bifrost, the active embedding representation is
EmbeddingsByTypewithFloat,Int8,Uint8,Binary,Ubinary, andBase64fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/invoke.go` around lines 1002 - 1019, The shim always reads d.Embedding.Float and emits a float-only envelope (BedrockInvokeCohereEmbeddingResp with ResponseType "embeddings_floats") even when the provider populated other types; update the embedding response construction in the Titan/resp.Data path to inspect EmbeddingsByType (e.g., check resp.Data[*].Embedding.EmbeddingsByType and its Float, Int8, Uint8, Binary, Ubinary, Base64 fields) and branch accordingly to return the matching native envelope (preserving Int8/Uint8/Binary/Ubinary/Base64 formats) or return an explicit error instead of always flattening to float; adjust the code paths that currently reference d.Embedding.Float and resp.Data[0].Embedding.Float to use the correct EmbeddingsByType fields and set ResponseType to the correct native type when building BedrockInvokeCohereEmbeddingResp or BedrockInvokeEmbeddingResp.ui/app/workspace/logs/sheets/logDetailView.tsx (1)
947-953:⚠️ Potential issue | 🟠 MajorUse the
embeddingobject key in the copy-request-body path.Line 952 still derives
isEmbeddingfrom"list", but this file renders embedding inputs behindlog.object === "embedding". As written, real embedding logs will skip the newlog.embedding_inputcopy branch, while list-model logs can be misclassified instead.Suggested fix
- const isEmbedding = log.object === "list"; + const isEmbedding = log.object === "embedding";Also applies to: 1016-1018
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/sheets/logDetailView.tsx` around lines 947 - 953, The isEmbedding detection currently derives from log.object === "list", causing real embedding logs to be missed and list-model logs misclassified; update the isEmbedding checks (the variable named isEmbedding and any similar checks around the copy-request-body branch that reference log.object) to use log.object === "embedding" (and include the chunk variant if needed, e.g., "embedding.chunk") so the copy button uses log.embedding_input for true embedding logs; apply the same replacement at the other occurrence near the copy-request-body logic (the block around the second isEmbedding declaration).core/providers/gemini/types.go (1)
1255-1279:⚠️ Potential issue | 🟠 Major
file_datasnake_case support is only half-implemented.This new fallback recognizes top-level
file_data, but it still unmarshals intoFileData, whose fields only acceptfileUri/mimeType/displayName. A Python-SDK payload likefile_data: {"file_uri":"...","mime_type":"application/pdf"}will therefore produce a non-nilFileDatawith empty fields, so file-based multimodal inputs still fail after this branch. Please add snake_case handling for the nestedFileDataobject as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/types.go` around lines 1255 - 1279, The code only falls back to top-level file_data into aux.FileDataSnake but does not translate nested snake_case fields into the canonical FileData, so Python-style payloads like file_data: {"file_uri": "...", "mime_type": "..."} end up as empty fields. Update the unmarshal/assignment logic for PartAlias/FileData handling: add a FileDataSnake struct (or ensure aux.FileDataSnake exists) that defines snake_case fields (file_uri, mime_type, display_name), then after sonic.Unmarshal map those snake_case fields into the canonical FileData fields (e.g., p.FileData.FileUri = aux.FileDataSnake.FileUri, p.FileData.MimeType = aux.FileDataSnake.MimeType, p.FileData.DisplayName = aux.FileDataSnake.DisplayName) when p.FileData is nil so nested snake_case payloads are correctly populated; keep references to PartAlias, FileData, FileDataSnake, p.FileData and sonic.Unmarshal to locate where to modify the code.
🧹 Nitpick comments (1)
plugins/semanticcache/test_utils.go (1)
555-562: Usebifrost.Ptr(...)for pointer creation consistency.At Line 559, pointer creation can follow repo convention for readability and consistency.
♻️ Proposed refactor
Input: func() *schemas.EmbeddingInput { contents := make([]schemas.EmbeddingContent, len(texts)) for i, text := range texts { - t := text - contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: &t}} + contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: bifrost.Ptr(text)}} } return &schemas.EmbeddingInput{Contents: contents} }(),Based on learnings: In maximhq/bifrost, prefer using
bifrost.Ptr()instead of&valuefor pointer construction consistency across code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/test_utils.go` around lines 555 - 562, Replace direct address-of usages when building the EmbeddingInput contents with the repo helper: in the anonymous Input func that returns *schemas.EmbeddingInput (the block creating contents := make([]schemas.EmbeddingContent, len(texts)) and assigning contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: &t}}), use bifrost.Ptr(t) instead of &t so the Text pointer is created via bifrost.Ptr(...) for consistency with project conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/internal/llmtests/embedding_multimodal.go`:
- Around line 107-110: The skip logic uses vertexNoBatch := testConfig.Provider
== schemas.Vertex which incorrectly prevents BatchImages/BatchMultimodal tests
for all Vertex configs; change the guard to target only the single-content
Gemini embedding route (e.g., check the model or a single-content flag instead
of Provider). Update vertexNoBatch to something like: vertexNoBatch :=
testConfig.Provider == schemas.Vertex && (testConfig.Model ==
"multimodalembedding@001" || testConfig.SingleContentGemini == true), and apply
the same change at the other occurrence (around the 208-210 block) so batch
image coverage still runs for Vertex multimodal models that support per-input
batching.
In `@core/internal/llmtests/response_validation.go`:
- Around line 1324-1326: The dimension-check logic incorrectly treats Base64
embeddings as empty because it manually inspects embedding.Embedding fields;
replace those manual checks with the existing getEmbeddingVector helper (from
core/internal/llmtests/utils.go) to extract the vector and then test len(vector)
> 0 and use that length for expected_dimensions; update all occurrences where
similar manual checks are used (the current hasData/embedding dimension
branches, and the other similar blocks noted) to call
getEmbeddingVector(embedding) instead of inspecting Base64/Binary/Int8/Uint8
fields directly so Base64 is counted and Binary/Ubinary remain intentionally
skipped.
In `@core/providers/bedrock/invoke.go`:
- Around line 397-409: The embedding request builder currently only sets
req.Input.Contents for r.InputText and r.Texts, leaving req.Input nil for
image-only or mixed requests; update the code in the embedding path that creates
req.Input so it also iterates r.Images and r.Inputs and appends corresponding
schemas.EmbeddingContent entries (using the proper EmbeddingContentPartType for
images/inputs and assigning the image URL or input payload pointers) so
req.Input is always non-nil when any of r.InputText, r.Texts, r.Images, or
r.Inputs are present; modify the section that constructs contents (the block
that sets contents := make([]schemas.EmbeddingContent, ...)) to include entries
for r.Images and r.Inputs and then set req.Input =
&schemas.EmbeddingInput{Contents: contents}.
In `@core/providers/gemini/embedding.go`:
- Around line 247-254: The current usage accounting reads only
embeddings[0].Statistics.TokenCount; update the logic in embedding.go (around
where geminiResp, embeddings and bifrostResp.Usage are handled) to sum
TokenCount across all embeddings that have Statistics (e.g., iterate embeddings
and accumulate Statistics.TokenCount when non-nil), and only if no per-embedding
statistics exist (sum == 0) fall back to
geminiResp.Metadata.BillableCharacterCount; then assign that summed value to
bifrostResp.Usage.PromptTokens and bifrostResp.Usage.TotalTokens so batched
embeddings are correctly accounted for.
- Around line 383-391: The loop currently overwrites bifrostReq.Params per item
so only the last item's embedding params survive; instead compute the params for
the first item (using applyBifrostEmbeddingParams) as the baseline, then for
each subsequent request compute its params and compare to the baseline; if any
mismatch is found, return an error (reject the batch) or only set
bifrostReq.Params if all items' params are identical. Reference the
request.Requests loop, geminiContentToEmbeddingContent,
applyBifrostEmbeddingParams, and bifrostReq.Params when locating where to
implement the baseline computation, per-item comparison, and early rejection.
In `@core/providers/vertex/embedding.go`:
- Around line 136-167: The loop that builds each multimodal instance currently
overwrites earlier parts (instance.Text, instance.Image, instance.Video) instead
of aggregating; change it to concatenate text parts (e.g., join successive
schemas.EmbeddingContentPartTypeText parts into instance.Text with a separator
like "\n\n") and treat image/video parts deterministically: if you want to
reject duplicates, return an error when instance.Image or instance.Video is
already set (with a clear message referencing
VertexMultimodalImageInput/VertexMultimodalVideoInput), or implement an explicit
merge strategy (e.g., collect multiple inputs into slices) rather than assigning
over them; update the switch handling of schemas.EmbeddingContentPartTypeImage
and ...Video (including use of extractBase64FromDataURI) to check for existing
instance.Image/instance.Video before assigning, and ensure the final instances =
append(instances, instance) receives the aggregated instance.
- Around line 212-237: The current converter emits multiple EmbeddingData
entries per `prediction` (from `prediction.TextEmbedding`,
`prediction.ImageEmbedding`, and `prediction.VideoEmbeddings`), breaking the 1
content → 1 embedding contract; update the code that builds `embeddings` (the
block creating schemas.EmbeddingData from `prediction`) so it produces exactly
one EmbeddingData per `prediction`: either (A) merge modality vectors into a
single vector (e.g., concatenate or compute a fixed-size average) and set that
as the single `Embedding` for the `EmbeddingData`, or (B) pick a single modality
by priority (e.g., prefer `TextEmbedding` then `ImageEmbedding` then the first
`VideoEmbeddings` entry) and use only that vector; implement the chosen strategy
where the current multiple appends occur (refer to `prediction.TextEmbedding`,
`prediction.ImageEmbedding`, `prediction.VideoEmbeddings`, `embeddings`, and
`schemas.EmbeddingData`) and ensure `idx` is incremented exactly once per
`prediction`; alternatively, if you opt to reject mixed-modality responses,
return an explicit error upstream when more than one modality is present instead
of appending multiple rows.
In `@core/providers/vertex/vertex.go`:
- Around line 1522-1540: After converting provider responses to bifrostResponse,
guard against nil before populating ExtraFields: check if bifrostResponse == nil
after calling gemini.ToBifrostEmbeddingResponse(...) and after
vertexResponse.ToBifrostEmbeddingResponse(), and if nil return a provider error
via providerUtils.EnrichError(providerUtils.NewBifrostOperationError(...)) (use
an appropriate schemas.ErrProviderResponse* code for "no embeddings/malformed
response") passing ctx, jsonBody, responseBody, provider.sendBackRawRequest and
provider.sendBackRawResponse so you don't dereference
bifrostResponse.ExtraFields on malformed-but-200 responses.
In `@core/schemas/embedding.go`:
- Around line 66-80: The EmbeddingMediaPart.Validate currently only checks for
non-nil Data/URL and allows empty payloads; update EmbeddingMediaPart.Validate
to treat empty values as unset by verifying that Data and URL are not only
non-nil but also contain non-zero length (e.g., check len of the underlying
string/byte slice pointed to by Data and the string pointed to by URL) and only
increment set when they are non-empty; if neither is non-empty or more than one
is set, return the existing error about exactly one of data or url being set (or
a similar clear error) so blank media payloads are rejected centrally.
- Around line 29-31: The EmbeddingInput struct's Contents field lacks a JSON tag
and therefore marshals as "Contents" instead of the expected snake_case
"contents"; update the EmbeddingInput definition by adding a JSON struct tag
(e.g., `json:"contents"` or `json:"contents,omitempty"`) to the Contents field
so it serializes as "contents" consistently across the schema (reference the
EmbeddingInput type and its Contents field).
In `@framework/logstore/tables.go`:
- Line 186: The EmbeddingInputParsed field is typed as schemas.EmbeddingContent
which doesn't exist; update the type to the actual exported embedding-content
type from core/schemas and adjust the import so the file uses that package
(e.g., import core/schemas package and replace schemas.EmbeddingContent with the
exported name from core/schemas). Ensure the field declaration
EmbeddingInputParsed []<CorrectExportedType> `gorm:"-"
json:"embedding_input,omitempty"` uses the exact exported identifier and that
any references to schemas in this file are updated to the correct package alias.
In `@plugins/semanticcache/utils.go`:
- Around line 450-459: The loop that builds textParts from
req.EmbeddingRequest.Input.Contents currently returns the raw joined string,
skipping normalizeText() and causing inconsistent caching; update this path to
normalize the text the same way as getNormalizedInputForCaching() by applying
normalizeText() (either to each *part.Text before appending to textParts or to
the final strings.Join(textParts, " ")) so the returned string is the normalized
value used for hashing and embedding lookup.
In `@tests/integrations/python/tests/test_google.py`:
- Around line 25-27: The module docstring's numbered list has drifted (14 → 45 →
46 → 15); update the two inserted items titled "Multimodal embedding - text +
image content (Gemini/Vertex gemini-embedding-2-preview)" and "Multimodal
embedding - batch contents (Gemini/Vertex gemini-embedding-2-preview)" to follow
the sequence (change 45 → 15 and 46 → 16) and then adjust any subsequent list
entries (e.g., the existing "15. List models") so numbering remains monotonic
(increment following items as needed).
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 1085-1089: Remove the early transport-side validation that checks
req.Input == nil || len(req.Input.Contents) == 0 and the call to
req.Input.Validate() in the inference HTTP handler; this validation is owned by
core/bifrost.go which implements the large-payload passthrough exception. Either
delete these two checks from the top of the handler or, if you must validate at
transport layer, move the checks to after the Bifrost context conversion step
and mirror the core passthrough logic (allow large/multimodal inputs through
when the Bifrost context indicates passthrough) so the handler forwards inputs
as-is and lets provider/core rejections propagate.
---
Outside diff comments:
In `@core/providers/bedrock/invoke.go`:
- Around line 1002-1019: The shim always reads d.Embedding.Float and emits a
float-only envelope (BedrockInvokeCohereEmbeddingResp with ResponseType
"embeddings_floats") even when the provider populated other types; update the
embedding response construction in the Titan/resp.Data path to inspect
EmbeddingsByType (e.g., check resp.Data[*].Embedding.EmbeddingsByType and its
Float, Int8, Uint8, Binary, Ubinary, Base64 fields) and branch accordingly to
return the matching native envelope (preserving Int8/Uint8/Binary/Ubinary/Base64
formats) or return an explicit error instead of always flattening to float;
adjust the code paths that currently reference d.Embedding.Float and
resp.Data[0].Embedding.Float to use the correct EmbeddingsByType fields and set
ResponseType to the correct native type when building
BedrockInvokeCohereEmbeddingResp or BedrockInvokeEmbeddingResp.
In `@core/providers/gemini/types.go`:
- Around line 1255-1279: The code only falls back to top-level file_data into
aux.FileDataSnake but does not translate nested snake_case fields into the
canonical FileData, so Python-style payloads like file_data: {"file_uri": "...",
"mime_type": "..."} end up as empty fields. Update the unmarshal/assignment
logic for PartAlias/FileData handling: add a FileDataSnake struct (or ensure
aux.FileDataSnake exists) that defines snake_case fields (file_uri, mime_type,
display_name), then after sonic.Unmarshal map those snake_case fields into the
canonical FileData fields (e.g., p.FileData.FileUri = aux.FileDataSnake.FileUri,
p.FileData.MimeType = aux.FileDataSnake.MimeType, p.FileData.DisplayName =
aux.FileDataSnake.DisplayName) when p.FileData is nil so nested snake_case
payloads are correctly populated; keep references to PartAlias, FileData,
FileDataSnake, p.FileData and sonic.Unmarshal to locate where to modify the
code.
In `@ui/app/workspace/logs/sheets/logDetailView.tsx`:
- Around line 947-953: The isEmbedding detection currently derives from
log.object === "list", causing real embedding logs to be missed and list-model
logs misclassified; update the isEmbedding checks (the variable named
isEmbedding and any similar checks around the copy-request-body branch that
reference log.object) to use log.object === "embedding" (and include the chunk
variant if needed, e.g., "embedding.chunk") so the copy button uses
log.embedding_input for true embedding logs; apply the same replacement at the
other occurrence near the copy-request-body logic (the block around the second
isEmbedding declaration).
---
Nitpick comments:
In `@plugins/semanticcache/test_utils.go`:
- Around line 555-562: Replace direct address-of usages when building the
EmbeddingInput contents with the repo helper: in the anonymous Input func that
returns *schemas.EmbeddingInput (the block creating contents :=
make([]schemas.EmbeddingContent, len(texts)) and assigning contents[i] =
schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text:
&t}}), use bifrost.Ptr(t) instead of &t so the Text pointer is created via
bifrost.Ptr(...) for consistency with project conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad84d434-521e-4a4d-8678-782f164b7906
📒 Files selected for processing (51)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/embedding.gocore/internal/llmtests/embedding_multimodal.gocore/internal/llmtests/response_validation.gocore/internal/llmtests/tests.gocore/internal/llmtests/utils.gocore/providers/azure/azure.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/cohere/cohere_test.gocore/providers/cohere/embedding.gocore/providers/cohere/embedding_multimodal_test.gocore/providers/cohere/types.gocore/providers/fireworks/fireworks_test.gocore/providers/gemini/embedding.gocore/providers/gemini/embedding_multimodal_test.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/types.gocore/providers/huggingface/embedding.gocore/providers/huggingface/huggingface_test.gocore/providers/openai/embedding.gocore/providers/openai/openai.gocore/providers/openai/types.gocore/providers/vertex/embedding.gocore/providers/vertex/embedding_multimodal_test.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vertex/vertex_test.gocore/schemas/embedding.gocore/schemas/embedding_multimodal_test.gocore/schemas/serialization_test.gocore/schemas/trace.goframework/logstore/migrations.goframework/logstore/tables.goframework/tracing/llmspan.goplugins/logging/main.goplugins/logging/utils.goplugins/logging/writer.goplugins/semanticcache/go.modplugins/semanticcache/plugin_core_test.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotests/integrations/python/config.ymltests/integrations/python/tests/test_cohere.pytests/integrations/python/tests/test_google.pytransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/cohere.goui/app/workspace/logs/sheets/logDetailView.tsxui/lib/types/logs.ts
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_core_test.go
✅ Files skipped from review due to trivial changes (12)
- core/schemas/embedding_multimodal_test.go
- core/providers/azure/azure.go
- core/providers/cohere/types.go
- core/providers/vertex/embedding_multimodal_test.go
- framework/logstore/migrations.go
- plugins/semanticcache/go.mod
- core/providers/gemini/embedding_multimodal_test.go
- core/providers/cohere/embedding_multimodal_test.go
- core/providers/gemini/gemini.go
- tests/integrations/python/tests/test_cohere.py
- core/providers/gemini/gemini_test.go
- core/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/logging/writer.go
- core/internal/llmtests/embedding.go
- core/internal/llmtests/tests.go
- core/internal/llmtests/utils.go
- plugins/logging/main.go
- core/providers/cohere/cohere_test.go
- core/providers/vertex/vertex_test.go
- core/internal/llmtests/account.go
- core/providers/fireworks/fireworks_test.go
- tests/integrations/python/config.yml
- core/providers/huggingface/huggingface_test.go
- core/providers/openai/embedding.go
- core/schemas/serialization_test.go
- core/providers/cohere/embedding.go
- core/providers/openai/openai.go
- core/providers/bedrock/embedding.go
4f21ee8 to
258c523
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/invoke.go (2)
413-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap
max_tokensandtruncateonto typed embedding params.These fields are only forwarded via
ExtraParamshere, butcohere.ToCohereEmbeddingRequest()readsParams.MaxTokensandParams.Truncate. Bedrock-native Cohere embedding requests therefore drop both settings on the provider hop.🛠️ Suggested fix
if r.Truncate != nil { - extraParams["truncate"] = *r.Truncate + // handled via typed params below } if len(r.Images) > 0 { extraParams["images"] = r.Images } if len(r.Inputs) > 0 { extraParams["inputs"] = r.Inputs } if r.MaxTokens != nil { - extraParams["max_tokens"] = *r.MaxTokens + // handled via typed params below } @@ params := &schemas.EmbeddingParameters{ Dimensions: dimensions, + MaxTokens: r.MaxTokens, + Truncate: r.Truncate, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/invoke.go` around lines 413 - 452, The code currently only forwards MaxTokens and Truncate via extraParams, but cohere.ToCohereEmbeddingRequest expects typed fields on schemas.EmbeddingParameters; update the mapping so that if r.MaxTokens != nil set params.MaxTokens = *r.MaxTokens and if r.Truncate != nil set params.Truncate = *r.Truncate (in addition to keeping them in extraParams if desired) before assigning req.Params, ensuring schemas.EmbeddingParameters carries these typed values for downstream cohere conversion.
999-1011:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve typed Cohere embeddings in native Bedrock responses.
This branch always emits
embeddings_floatsfromd.Embedding.Float. If the Bifrost response carriesBase64,Int8,Uint8,Binary, orUbinary, Bedrock clients either lose the requested type or receive empty float vectors instead of the nativeembeddings_by_typeenvelope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/invoke.go` around lines 999 - 1011, The current branch unconditionally converts embeddings from resp.Data using d.Embedding.Float and returns a BedrockInvokeCohereEmbeddingResp with ResponseType "embeddings_floats"; instead, detect which embedding field is present on each resp.Data item (e.g., d.Embedding.Float, d.Embedding.Base64, d.Embedding.Int8, d.Embedding.Uint8, d.Embedding.Binary, d.Embedding.Ubinary) and preserve the native typed data in the response envelope rather than forcing float conversion—update the logic around resp.Data iteration to build a payload that keeps the original type and set ResponseType to "embeddings_by_type" (or the appropriate native-type tag) when non-float embeddings are present so Bedrock clients receive the original typed embeddings intact while still falling back to the float path for Float embeddings; ensure you return a BedrockInvokeCohereEmbeddingResp that contains the corresponding typed field(s) rather than always populating Embeddings from d.Embedding.Float.
♻️ Duplicate comments (1)
core/internal/llmtests/response_validation.go (1)
1345-1360:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Base64embeddings still validate as 0-dimensional.These switches now accept
Base64as real embedding data indirectly via the earlierhasDatacheck, but they never count it here. Any provider returning base64-encoded embeddings will failexpected_dimensions, andembedding_dimensionswill be recorded as0even though the embedding is valid. ReusegetEmbeddingVector(...)in both places so validation and metrics stay aligned with the active embedding representations.Proposed fix
if expectedDimensions, ok := expectations.ProviderSpecific["expected_dimensions"].(int); ok { for i, embedding := range response.Data { - e := embedding.Embedding - var actualDimensions int - switch { - case len(e.Float) > 0: - actualDimensions = len(e.Float) - case len(e.Int8) > 0: - actualDimensions = len(e.Int8) - case len(e.Uint8) > 0: - actualDimensions = len(e.Uint8) - case len(e.Binary) > 0: - actualDimensions = len(e.Binary) - case len(e.Ubinary) > 0: - actualDimensions = len(e.Ubinary) - } + vec, err := getEmbeddingVector(embedding) + if err != nil { + result.Passed = false + result.Errors = append(result.Errors, + fmt.Sprintf("Embedding %d: failed to extract vector: %v", i, err)) + continue + } + actualDimensions := len(vec) if actualDimensions != expectedDimensions { result.Passed = false result.Errors = append(result.Errors, fmt.Sprintf("Embedding %d has %d dimensions, expected %d", i, actualDimensions, expectedDimensions)) } @@ result.MetricsCollected["has_data"] = response.Data != nil result.MetricsCollected["embedding_count"] = len(response.Data) result.MetricsCollected["has_usage"] = response.Usage != nil if len(response.Data) > 0 { - e := response.Data[0].Embedding - var dimensions int - switch { - case len(e.Float) > 0: - dimensions = len(e.Float) - case len(e.Int8) > 0: - dimensions = len(e.Int8) - case len(e.Uint8) > 0: - dimensions = len(e.Uint8) - case len(e.Binary) > 0: - dimensions = len(e.Binary) - case len(e.Ubinary) > 0: - dimensions = len(e.Ubinary) + if vec, err := getEmbeddingVector(response.Data[0]); err == nil { + result.MetricsCollected["embedding_dimensions"] = len(vec) } - result.MetricsCollected["embedding_dimensions"] = dimensions } }Also applies to: 1530-1545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/internal/llmtests/response_validation.go` around lines 1345 - 1360, The expected-dimensions check currently inspects only Float/Int8/Uint8/Binary/Ubinary fields and thus treats Base64 embeddings as zero-length; update the logic that computes actualDimensions (the block inside the if for expectations.ProviderSpecific["expected_dimensions"] and the similar block around lines 1530-1545) to call and reuse getEmbeddingVector(...) to derive the embedding vector length instead of the manual switch, so both validation and recorded embedding_dimensions use the same representation; use the returned slice length from getEmbeddingVector(response.Data[i].Embedding) (handling any error/ok result per existing helper convention) to set actualDimensions and keep metric/validation consistent.
🧹 Nitpick comments (1)
core/providers/openai/types.go (1)
134-135: 💤 Low valueUpdate the outdated inline comment.
The comment states "Can be string or []string" but
OpenAIEmbeddingInputnow supports four shapes:string,[]string,[]int, and[][]int.📝 Suggested documentation fix
type OpenAIEmbeddingRequest struct { Model string `json:"model"` - Input *OpenAIEmbeddingInput `json:"input"` // Can be string or []string + Input *OpenAIEmbeddingInput `json:"input"` // One-of: string, []string, []int (tokens), or [][]int (token batches)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/types.go` around lines 134 - 135, The inline comment on the Input field of the embedding request is outdated; update the comment for Input *OpenAIEmbeddingInput in core/providers/openai/types.go (next to the Model field) to list all supported shapes: string, []string, []int, and [][]int (or similar phrasing) so it accurately documents the OpenAIEmbeddingInput type's four accepted forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/providers/bedrock/invoke.go`:
- Around line 413-452: The code currently only forwards MaxTokens and Truncate
via extraParams, but cohere.ToCohereEmbeddingRequest expects typed fields on
schemas.EmbeddingParameters; update the mapping so that if r.MaxTokens != nil
set params.MaxTokens = *r.MaxTokens and if r.Truncate != nil set params.Truncate
= *r.Truncate (in addition to keeping them in extraParams if desired) before
assigning req.Params, ensuring schemas.EmbeddingParameters carries these typed
values for downstream cohere conversion.
- Around line 999-1011: The current branch unconditionally converts embeddings
from resp.Data using d.Embedding.Float and returns a
BedrockInvokeCohereEmbeddingResp with ResponseType "embeddings_floats"; instead,
detect which embedding field is present on each resp.Data item (e.g.,
d.Embedding.Float, d.Embedding.Base64, d.Embedding.Int8, d.Embedding.Uint8,
d.Embedding.Binary, d.Embedding.Ubinary) and preserve the native typed data in
the response envelope rather than forcing float conversion—update the logic
around resp.Data iteration to build a payload that keeps the original type and
set ResponseType to "embeddings_by_type" (or the appropriate native-type tag)
when non-float embeddings are present so Bedrock clients receive the original
typed embeddings intact while still falling back to the float path for Float
embeddings; ensure you return a BedrockInvokeCohereEmbeddingResp that contains
the corresponding typed field(s) rather than always populating Embeddings from
d.Embedding.Float.
---
Duplicate comments:
In `@core/internal/llmtests/response_validation.go`:
- Around line 1345-1360: The expected-dimensions check currently inspects only
Float/Int8/Uint8/Binary/Ubinary fields and thus treats Base64 embeddings as
zero-length; update the logic that computes actualDimensions (the block inside
the if for expectations.ProviderSpecific["expected_dimensions"] and the similar
block around lines 1530-1545) to call and reuse getEmbeddingVector(...) to
derive the embedding vector length instead of the manual switch, so both
validation and recorded embedding_dimensions use the same representation; use
the returned slice length from getEmbeddingVector(response.Data[i].Embedding)
(handling any error/ok result per existing helper convention) to set
actualDimensions and keep metric/validation consistent.
---
Nitpick comments:
In `@core/providers/openai/types.go`:
- Around line 134-135: The inline comment on the Input field of the embedding
request is outdated; update the comment for Input *OpenAIEmbeddingInput in
core/providers/openai/types.go (next to the Model field) to list all supported
shapes: string, []string, []int, and [][]int (or similar phrasing) so it
accurately documents the OpenAIEmbeddingInput type's four accepted forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8894dcf-a494-49dc-a489-43c10a4908c1
📒 Files selected for processing (34)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/embedding.gocore/internal/llmtests/embedding_multimodal.gocore/internal/llmtests/response_validation.gocore/internal/llmtests/tests.gocore/internal/llmtests/utils.gocore/providers/azure/azure.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/cohere/cohere_test.gocore/providers/cohere/embedding.gocore/providers/cohere/embedding_multimodal_test.gocore/providers/cohere/types.gocore/providers/fireworks/fireworks_test.gocore/providers/gemini/embedding.gocore/providers/gemini/embedding_multimodal_test.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/types.gocore/providers/huggingface/embedding.gocore/providers/huggingface/huggingface_test.gocore/providers/openai/embedding.gocore/providers/openai/openai.gocore/providers/openai/types.gocore/providers/vertex/embedding.gocore/providers/vertex/embedding_multimodal_test.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vertex/vertex_test.gocore/schemas/embedding.gocore/schemas/embedding_multimodal_test.gocore/schemas/serialization_test.gocore/schemas/trace.go
✅ Files skipped from review due to trivial changes (12)
- core/providers/cohere/types.go
- core/schemas/embedding_multimodal_test.go
- core/providers/vertex/embedding_multimodal_test.go
- core/internal/llmtests/embedding.go
- core/providers/openai/openai.go
- core/providers/gemini/types.go
- core/providers/vertex/vertex_test.go
- core/providers/cohere/embedding_multimodal_test.go
- core/providers/gemini/gemini_test.go
- core/schemas/embedding.go
- core/bifrost.go
- core/providers/gemini/embedding.go
🚧 Files skipped from review as they are similar to previous changes (9)
- core/providers/gemini/embedding_multimodal_test.go
- core/providers/huggingface/huggingface_test.go
- core/providers/fireworks/fireworks_test.go
- core/providers/openai/embedding.go
- core/providers/gemini/gemini.go
- core/internal/llmtests/tests.go
- core/internal/llmtests/utils.go
- core/schemas/trace.go
- core/providers/vertex/vertex.go
f42eac2 to
f83ea4a
Compare
258c523 to
3ce9372
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/gemini/types.go (1)
1267-1292:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
file_datasnake_case support still drops nested fields.This fallback only switches the outer key from
fileDatatofile_data. If the nested object also uses snake_case (file_uri,mime_type,display_name),FileDatastill unmarshals to empty fields because it only has camelCase tags. That means Python-SDK-style file parts still fail on this path.💡 Suggested follow-up
type FileData struct { DisplayName string `json:"displayName,omitempty"` FileURI string `json:"fileUri,omitempty"` MIMEType string `json:"mimeType,omitempty"` } + +func (f *FileData) UnmarshalJSON(data []byte) error { + type alias struct { + DisplayName string `json:"displayName,omitempty"` + DisplayNameSnake string `json:"display_name,omitempty"` + FileURI string `json:"fileUri,omitempty"` + FileURISnake string `json:"file_uri,omitempty"` + MIMEType string `json:"mimeType,omitempty"` + MIMETypeSnake string `json:"mime_type,omitempty"` + } + + var aux alias + if err := sonic.Unmarshal(data, &aux); err != nil { + return err + } + + if aux.DisplayName != "" { + f.DisplayName = aux.DisplayName + } else { + f.DisplayName = aux.DisplayNameSnake + } + if aux.FileURI != "" { + f.FileURI = aux.FileURI + } else { + f.FileURI = aux.FileURISnake + } + if aux.MIMEType != "" { + f.MIMEType = aux.MIMEType + } else { + f.MIMEType = aux.MIMETypeSnake + } + return nil +}
♻️ Duplicate comments (3)
core/providers/gemini/embedding.go (1)
396-407:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t collapse mixed per-item params onto the first batch entry.
BifrostEmbeddingRequestonly has one sharedParams, but this path always derives it fromrequest.Requests[0]. If later Gemini batch items carry differenttaskType,title,outputDimensionality,documentOcr, oraudioTrackExtraction, that information is silently dropped during reverse conversion. Either verify all entries agree before collapsing them, or reject heterogeneous batches here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/embedding.go` around lines 396 - 407, The conversion in the Gemini->Bifrost path currently collapses per-item params into a single Params using applyBifrostEmbeddingParams(bifrostReq.Params, request.Requests[0]) which silently drops differing per-request fields; update the logic in the function handling request.Requests so that before collapsing you iterate request.Requests and verify that taskType, title, outputDimensionality, documentOcr, and audioTrackExtraction (the per-item fields) are identical across all entries — if they are identical set bifrostReq.Params once from any entry, otherwise return an error/reject the batch (or alternatively return a clear validation error) so heterogeneous batches are not silently lost; reference geminiContentToEmbeddingContent and BifrostEmbeddingRequest when implementing the check.core/providers/vertex/embedding.go (2)
212-237:⚠️ Potential issue | 🟠 MajorEmit exactly one embedding row per prediction.
This block can append a text embedding, an image embedding, and multiple video embeddings for a single
prediction, which breaks the 1 content -> 1 output invariant and misalignsData[index]with the originalInput.Contentsentry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/vertex/embedding.go` around lines 212 - 237, The code currently appends multiple embedding rows per prediction (text, image, and every video), breaking the 1 content -> 1 output invariant; change the logic in the embedding construction (the block that references prediction.TextEmbedding, prediction.ImageEmbedding, prediction.VideoEmbeddings, schemas.EmbeddingData and idx) to pick exactly one embedding per prediction—e.g., choose prediction.TextEmbedding if present, else prediction.ImageEmbedding, else the first entry of prediction.VideoEmbeddings—and append a single schemas.EmbeddingData with that chosen vector and increment idx once. Ensure you copy the chosen slice into EmbeddingsByType{Float: append([]float64(nil), ...)} as before and remove the multiple appends/loop.
136-167:⚠️ Potential issue | 🟠 MajorAggregate parts instead of overwriting them.
One
EmbeddingContentis supposed to aggregate its parts into one embedding, butinstance.Text = part.Text,instance.Image = img, andinstance.Video = vidkeep only the last value of each modality. Earlier text fragments or duplicate media parts are silently discarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/vertex/embedding.go` around lines 136 - 167, The loop currently overwrites instance.Text/instance.Image/instance.Video for each part, losing earlier parts; change it to aggregate parts: concatenate text fragments into instance.Text (e.g., append with separator or join), and collect image and video inputs into slices (e.g., instance.Images []VertexMultimodalImageInput and instance.Videos []VertexMultimodalVideoInput) instead of single pointers; update the Vertex multimodal instance type and any downstream consumers that reference instance.Image/instance.Video to handle slices, and in the switch cases append the created img/vid values rather than assigning them.
🧹 Nitpick comments (2)
core/providers/openai/openai.go (1)
1960-1961: ⚡ Quick winLet core populate provider/model attribution fields.
ProviderandResolvedModelUsedare owned by the centralized extra-field population path for unary responses. Setting them here makes embeddings inconsistent with the other OpenAI handlers and risks freezing provider-layer values before the core enricher runs.Based on learnings:
Provider,OriginalModelRequested, andResolvedModelUsedonBifrostResponseExtraFieldsare populated centrally by(*BifrostResponse).PopulateExtraFieldsincore/bifrost.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/openai/openai.go` around lines 1960 - 1961, Remove the premature population of provider/model attribution in core/providers/openai/openai.go by deleting the assignments to response.ExtraFields.Provider and response.ExtraFields.ResolvedModelUsed (and any similar direct writes such as OriginalModelRequested) so the centralized population path can run; locate the block where response.ExtraFields is being set in the OpenAI handler and remove those provider/model assignments, leaving other extra-field writes intact and relying on (*BifrostResponse).PopulateExtraFields in core/bifrost.go to populate Provider, OriginalModelRequested, and ResolvedModelUsed.core/providers/azure/azure.go (1)
880-883: ⚡ Quick winLet core populate
ExtraFields.Provider.This field is supposed to be filled centrally on successful responses. Setting it in the provider duplicates that contract and makes this path easier to drift from the rest of the stack.
♻️ Suggested cleanup
response := openaiResp.ToBifrostEmbeddingResponse() - response.ExtraFields.Provider = provider.GetProviderKey() response.ExtraFields.Latency = latency.Milliseconds() response.ExtraFields.ProviderResponseHeaders = providerResponseHeadersBased on learnings: In maximhq/bifrost, the fields
OriginalModelRequested,Provider, andResolvedModelUsedonBifrostResponseExtraFieldsare populated centrally by(*BifrostResponse).PopulateExtraFieldsincore/bifrost.gofor all response paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/azure/azure.go` around lines 880 - 883, Remove the provider-level population of ExtraFields.Provider in the Azure embedding response path: after calling openaiResp.ToBifrostEmbeddingResponse() you should not set response.ExtraFields.Provider = provider.GetProviderKey(); leave other fields (e.g., response.ExtraFields.Latency = latency.Milliseconds()) intact. The core centralizer (*BifrostResponse).PopulateExtraFields is responsible for filling OriginalModelRequested, Provider, and ResolvedModelUsed, so delete the assignment of Provider here to avoid duplicating/contradicting that contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/gemini/embedding.go`:
- Around line 190-206: The loop is incorrectly copying the aggregate
bifrostResp.Usage.PromptTokens into each GeminiEmbedding.Statistics causing
overcounting; remove the per-item assignment inside the loop in embedding.go (do
not set ContentEmbeddingStatistics within the for i, embedding := range
bifrostResp.Data block) and instead only populate geminiResp.Embeddings with
Values there; after the loop, if there is exactly one embedding
(len(geminiResp.Embeddings) == 1) set geminiResp.Embedding and then assign
Statistics on that single embedding using bifrostResp.Usage.PromptTokens (via
ContentEmbeddingStatistics) while keeping the aggregate usage in
geminiResp.Metadata as you already do; do not create per-item Statistics from
aggregate Usage when batching.
In `@core/providers/openai/embedding.go`:
- Around line 135-160: The loop over each content entry can silently drop
entries when all parts have nil/empty Text and no Tokens, causing index
mismatches; after the inner loop that builds sb and tokens (the loop iterating
over part in content and using sb, tokens, texts, tokenBatches), add a guard: if
sb.Len() == 0 && len(tokens) == 0 return an error (e.g. fmt.Errorf("openai
embedding received empty content entry: all parts have nil/empty Text and no
Tokens")) so the caller is notified instead of silently discarding the input;
this mirrors the defensive behavior in extractTextFromContent used in the
HuggingFace converter.
In `@core/providers/openai/openai.go`:
- Around line 1945-1959: The large-payload branch must use the same
OpenAI→Bifrost conversion as the normal path: instead of unmarshalling directly
into schemas.BifrostEmbeddingResponse, unmarshal into OpenaiEmbeddingResponse
and call its ToBifrostEmbeddingResponse() to produce the Bifrost response; also
populate rawRequest/rawResponse via customResponseHandler or
providerUtils.HandleProviderResponse just like the normal branch, and use
providerUtils.EnrichError for error handling to keep schema and multimodal/typed
embedding fields consistent across both paths (refer to OpenaiEmbeddingResponse,
ToBifrostEmbeddingResponse, customResponseHandler,
providerUtils.HandleProviderResponse, and providerUtils.EnrichError).
In `@core/providers/vertex/embedding.go`:
- Around line 148-149: Validate that part.Image.URL and part.Video.URL start
with "gs://” before setting img.GCSUri or vid.GCSUri: use
strings.HasPrefix(*part.Image.URL, "gs://") and
strings.HasPrefix(*part.Video.URL, "gs://") and if the check fails return an
error (e.g., fmt.Errorf("image/video URL must be a GCS URI (gs://...)")) instead
of assigning; add the necessary strings import and apply the same validation
logic to both the image assignment (img.GCSUri) and the video assignment
(vid.GCSUri) so non-gs:// URIs are rejected locally rather than sent to Vertex.
In `@core/schemas/embedding.go`:
- Around line 226-232: The EmbeddingsByType struct currently declares Binary
[]int8 and Ubinary []uint8 which breaks the public contract; change these fields
to Binary []byte and Ubinary []byte (keeping their json tags and other fields
unchanged) so the struct matches the repo-wide contract and callers/converters
expecting packed-binary embeddings continue to work; update any related comments
or docs referencing Binary/Ubinary types if present.
- Around line 117-152: The EmbeddingContentPart validator currently only checks
p.Tokens for nil, allowing an empty slice (Tokens: []int{}) to pass; update the
EmbeddingContentPartTypeTokens branch in the Validate method to reject empty
token slices by checking len(p.Tokens) == 0 and returning an error like
"embedding content part type %q requires non-empty tokens payload" when
zero-length, instead of only checking for nil.
---
Duplicate comments:
In `@core/providers/gemini/embedding.go`:
- Around line 396-407: The conversion in the Gemini->Bifrost path currently
collapses per-item params into a single Params using
applyBifrostEmbeddingParams(bifrostReq.Params, request.Requests[0]) which
silently drops differing per-request fields; update the logic in the function
handling request.Requests so that before collapsing you iterate request.Requests
and verify that taskType, title, outputDimensionality, documentOcr, and
audioTrackExtraction (the per-item fields) are identical across all entries — if
they are identical set bifrostReq.Params once from any entry, otherwise return
an error/reject the batch (or alternatively return a clear validation error) so
heterogeneous batches are not silently lost; reference
geminiContentToEmbeddingContent and BifrostEmbeddingRequest when implementing
the check.
In `@core/providers/vertex/embedding.go`:
- Around line 212-237: The code currently appends multiple embedding rows per
prediction (text, image, and every video), breaking the 1 content -> 1 output
invariant; change the logic in the embedding construction (the block that
references prediction.TextEmbedding, prediction.ImageEmbedding,
prediction.VideoEmbeddings, schemas.EmbeddingData and idx) to pick exactly one
embedding per prediction—e.g., choose prediction.TextEmbedding if present, else
prediction.ImageEmbedding, else the first entry of
prediction.VideoEmbeddings—and append a single schemas.EmbeddingData with that
chosen vector and increment idx once. Ensure you copy the chosen slice into
EmbeddingsByType{Float: append([]float64(nil), ...)} as before and remove the
multiple appends/loop.
- Around line 136-167: The loop currently overwrites
instance.Text/instance.Image/instance.Video for each part, losing earlier parts;
change it to aggregate parts: concatenate text fragments into instance.Text
(e.g., append with separator or join), and collect image and video inputs into
slices (e.g., instance.Images []VertexMultimodalImageInput and instance.Videos
[]VertexMultimodalVideoInput) instead of single pointers; update the Vertex
multimodal instance type and any downstream consumers that reference
instance.Image/instance.Video to handle slices, and in the switch cases append
the created img/vid values rather than assigning them.
---
Nitpick comments:
In `@core/providers/azure/azure.go`:
- Around line 880-883: Remove the provider-level population of
ExtraFields.Provider in the Azure embedding response path: after calling
openaiResp.ToBifrostEmbeddingResponse() you should not set
response.ExtraFields.Provider = provider.GetProviderKey(); leave other fields
(e.g., response.ExtraFields.Latency = latency.Milliseconds()) intact. The core
centralizer (*BifrostResponse).PopulateExtraFields is responsible for filling
OriginalModelRequested, Provider, and ResolvedModelUsed, so delete the
assignment of Provider here to avoid duplicating/contradicting that contract.
In `@core/providers/openai/openai.go`:
- Around line 1960-1961: Remove the premature population of provider/model
attribution in core/providers/openai/openai.go by deleting the assignments to
response.ExtraFields.Provider and response.ExtraFields.ResolvedModelUsed (and
any similar direct writes such as OriginalModelRequested) so the centralized
population path can run; locate the block where response.ExtraFields is being
set in the OpenAI handler and remove those provider/model assignments, leaving
other extra-field writes intact and relying on
(*BifrostResponse).PopulateExtraFields in core/bifrost.go to populate Provider,
OriginalModelRequested, and ResolvedModelUsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d61229a-9fb6-4a1c-b16b-18d6d4c77bf9
📒 Files selected for processing (33)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/embedding.gocore/internal/llmtests/embedding_multimodal.gocore/internal/llmtests/response_validation.gocore/internal/llmtests/tests.gocore/internal/llmtests/utils.gocore/providers/azure/azure.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/cohere/cohere_test.gocore/providers/cohere/embedding.gocore/providers/cohere/embedding_multimodal_test.gocore/providers/cohere/types.gocore/providers/fireworks/fireworks_test.gocore/providers/gemini/embedding.gocore/providers/gemini/embedding_multimodal_test.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/types.gocore/providers/huggingface/embedding.gocore/providers/huggingface/huggingface_test.gocore/providers/mistral/custom_provider_test.gocore/providers/openai/embedding.gocore/providers/openai/openai.gocore/providers/openai/types.gocore/providers/vertex/embedding.gocore/providers/vertex/embedding_multimodal_test.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vertex/vertex_test.gocore/schemas/embedding.gocore/schemas/embedding_multimodal_test.go
✅ Files skipped from review due to trivial changes (8)
- core/providers/cohere/types.go
- core/schemas/embedding_multimodal_test.go
- core/internal/llmtests/tests.go
- core/providers/gemini/gemini.go
- core/providers/cohere/embedding_multimodal_test.go
- core/providers/huggingface/huggingface_test.go
- core/providers/gemini/embedding_multimodal_test.go
- core/providers/cohere/embedding.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/providers/vertex/embedding_multimodal_test.go
- core/internal/llmtests/account.go
- core/internal/llmtests/utils.go
- core/internal/llmtests/response_validation.go
- core/internal/llmtests/embedding_multimodal.go
- core/providers/vertex/vertex.go
- core/bifrost.go
3ce9372 to
dfd8f2b
Compare
| if part.Type != schemas.EmbeddingContentPartTypeText || part.Text == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Nil pointer wrapped in interface causes panic on non-text parts
When a non-text part is encountered, return nil returns a typed-nil *VertexEmbeddingRequest. At the vertex.go call site this value is returned as a providerUtils.RequestBodyWithExtraParams interface — which is non-nil (it has type *VertexEmbeddingRequest, value nil). CheckContextAndGetRequestBody therefore passes the nil-check at line 1132 and proceeds to call MarshalSortedIndent (producing null body) and, when BifrostContextKeyPassthroughExtraParams is set, calls convertedBody.GetExtraParams() which dereferences the nil receiver and panics. The function should return (*VertexEmbeddingRequest, error) and surface a descriptive error instead of returning nil.
| case len(req.Images) > 0: | ||
| url := req.Images[0] | ||
| bifrostReq.Input.Contents = []schemas.EmbeddingContent{{{ | ||
| Type: schemas.EmbeddingContentPartTypeImage, | ||
| Image: &schemas.EmbeddingMediaPart{URL: &url}, | ||
| }}} |
There was a problem hiding this comment.
Multiple images silently dropped in
ToBifrostEmbeddingRequest
When req.Images contains more than one URL, only req.Images[0] is placed into a single EmbeddingContent entry and all subsequent images are silently discarded. A Cohere request that arrives with e.g. "images": ["url1","url2","url3"] will be converted to a single-image Bifrost request, losing the remaining images entirely. Each image should map to its own EmbeddingContent entry, mirroring the existing Texts handling above.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/logging/main.go (1)
514-551:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
BatchEmbeddingRequeststill skips input logging.This new branch only populates
initialData.EmbeddingInputforschemas.EmbeddingRequest, so/v1/embeddings/batchrequests will still log no embedding input even when content logging is enabled. Please add aschemas.BatchEmbeddingRequestcase that extracts and redacts each item'sContentthe same way as the unary path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/main.go` around lines 514 - 551, Add a new case for schemas.BatchEmbeddingRequest mirroring the schemas.EmbeddingRequest branch: set initialData.Params = req.BatchEmbeddingRequest.Params, extract each item's embedding content (reuse or call the same helper used by EmbeddingRequest, e.g., extractEmbeddingInput but adapted for batch input), compute totalDataSize by iterating each item's media parts and redact large media via redactEmbeddingMediaData when threshold from ctx (schemas.BifrostContextKeyLargePayloadRequestThreshold) is exceeded, then assign the resulting slice to initialData.EmbeddingInput so batch embedding requests log their (possibly redacted) inputs.
♻️ Duplicate comments (4)
core/providers/openai/types.go (1)
1118-1161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reintroducing deprecated embedding wire model (
EmbeddingStruct/ array variants).At Line 1118 onward, this adds a legacy embedding representation that conflicts with the stack’s current embedding contract and risks divergent handling paths. Prefer a single provider wire shape (
[]float64) here and map toEmbeddingsByTypein converters.Suggested simplification
type EmbeddingData struct { Index int `json:"index"` Object string `json:"object"` // "embedding" - Embedding EmbeddingStruct `json:"embedding"` // can be string, []float64 or [][]float64 + Embedding []float64 `json:"embedding"` } - -type EmbeddingStruct struct { - EmbeddingStr *string - EmbeddingArray []float64 - Embedding2DArray [][]float64 -} - -func (be EmbeddingStruct) MarshalJSON() ([]byte, error) { ... } -func (be *EmbeddingStruct) UnmarshalJSON(data []byte) error { ... }Based on learnings: In maximhq/bifrost, the live embedding schema no longer contains
EmbeddingStructandEmbeddingArray/Embedding2DArray; the active representation isEmbeddingsByTypeincore/schemas/embedding.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/openai/types.go` around lines 1118 - 1161, The PR reintroduces a deprecated legacy embedding wire model (EmbeddingStruct with EmbeddingStr, EmbeddingArray, Embedding2DArray and its MarshalJSON/UnmarshalJSON) which conflicts with the current embedding contract; remove EmbeddingStruct and its custom JSON methods and instead ensure provider responses use the single canonical wire shape ([]float64) and map into the existing EmbeddingsByType conversion flow (see EmbeddingsByType in core/schemas/embedding.go) in the provider converters/adapter functions so all providers funnel through the same conversion path.core/providers/vertex/embedding.go (2)
215-240:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn one embedding per prediction, not one per modality.
Appending separate rows for
TextEmbedding,ImageEmbedding, and everyVideoEmbeddingbreaks the new 1-content → 1-embedding contract and will misalign indexes oncemultimodalembedding@001is exercised. If Vertex can return multiple modality vectors for one prediction, this layer needs to merge/select them or reject that response shape instead of fanning outresp.Data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/vertex/embedding.go` around lines 215 - 240, The code currently appends one EmbeddingData per modality (prediction.TextEmbedding, prediction.ImageEmbedding, prediction.VideoEmbeddings) which violates the 1-content→1-embedding contract; change the logic in the loop that builds embeddings so each prediction yields exactly one schemas.EmbeddingData (keep using the embeddings slice, schemas.EmbeddingData and idx), e.g. if prediction.TextEmbedding is non-empty choose that as the single embedding, else if prediction.ImageEmbedding is non-empty choose that, else if prediction.VideoEmbeddings has elements choose the first VideoEmbedding; construct one EmbeddingData with the selected vector (using append([]float64(nil), ...)) and increment idx once, or alternatively return an error when multiple modality vectors are present if you prefer to reject multimodal responses.
134-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't overwrite earlier parts inside one multimodal content entry.
This loop still keeps only the last text/image/video part for a given
EmbeddingContent, so earlier parts are silently dropped before the request is sent. Concatenate repeated text parts and either merge or reject duplicate image/video parts explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/vertex/embedding.go` around lines 134 - 166, The loop over bifrostReq.Input.Contents currently overwrites earlier parts in a single VertexEmbeddingInstance; fix by concatenating repeated text parts and explicitly handling duplicate image/video parts: when processing part.Type in the loop for each content, if part.Type is EmbeddingContentPartTypeText append part.Text to instance.Text (with a space or newline) instead of assigning, and for EmbeddingContentPartTypeImage / EmbeddingContentPartTypeVideo check if instance.Image or instance.Video is already set and either (a) return a clear error rejecting multiple image/video parts in one EmbeddingContent or (b) change VertexEmbeddingInstance to accept slices (e.g., Images []VertexMultimodalImageInput, Videos []VertexMultimodalVideoInput) and append new entries; apply the same payload validation (Data vs URL and gs:// checks) before appending, and ensure downstream request building uses the new concatenated/merged fields.tests/integrations/python/tests/test_cohere.py (1)
158-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate these Cohere-specific embedding features by capability.
test_06_custom_dimensions_embeddingandtest_07_multiple_embedding_typesstill run across everyembeddingsprovider/model, butoutput_dimension=512andint8embeddings are not universally supported. That makes these tests fail for capability gaps rather than transport regressions. Please add the same capability/skip guard discussed earlier, or scope these cases to the providers/models that actually support them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/python/tests/test_cohere.py` around lines 158 - 196, Both tests run against all embedding providers but rely on Cohere-only features (output_dimension=512 and int8 embeddings); add capability guards so they skip when the provider/model doesn't support those features. In test_06_custom_dimensions_embedding and test_07_multiple_embedding_types, call or implement capability checks before getting the client (e.g., a helper like supports_output_dimension(provider, model) and supports_embedding_type(provider, model, "int8") or reuse existing provider capability helpers) and call pytest.skip when unsupported; keep the rest of each test (client = get_provider_cohere_client(...), response = client.embed(...), and assertions) unchanged and reference the existing symbols test_06_custom_dimensions_embedding, test_07_multiple_embedding_types, get_provider_cohere_client, format_provider_model to locate where to insert the guard.
🧹 Nitpick comments (4)
plugins/semanticcache/test_utils.go (1)
553-556: ⚡ Quick winPrefer
bifrost.Ptrfor pointer construction in this helper.This keeps pointer creation consistent with repository conventions used across test utilities.
♻️ Suggested change
- for i, text := range texts { - t := text - contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: &t}} - } + for i, text := range texts { + contents[i] = schemas.EmbeddingContent{{Type: schemas.EmbeddingContentPartTypeText, Text: bifrost.Ptr(text)}} + }Based on learnings: “prefer using bifrost.Ptr() to create pointers instead of the address operator (&) … including test utilities.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/test_utils.go` around lines 553 - 556, Replace the use of the address operator when constructing EmbeddingContent parts with the repository helper; instead of using Text: &t in the loop that builds contents (the schemas.EmbeddingContent literal with Type: schemas.EmbeddingContentPartTypeText), call bifrost.Ptr(t) to create the pointer consistently, and add the bifrost import if missing.core/providers/cohere/embedding_test.go (1)
97-99: ⚡ Quick winStrengthen multimodal assertions with payload checks.
You already validate block types; add assertions for the mapped text/image payload values too, so regressions in content mapping are caught.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/cohere/embedding_test.go` around lines 97 - 99, Add assertions that validate the actual payload values for the mapped content blocks in addition to their types: after the existing assertions on CohereContentBlockTypeText and CohereContentBlockTypeImage, assert that req.Inputs[0].Content[0] contains the original text payload (e.g., the text field/value equals the text string used earlier in the test) and that req.Inputs[0].Content[1] contains the original image payload (e.g., the image URL or image field/value equals the image string used earlier in the test) so regressions in mapping are caught.core/providers/gemini/gemini.go (1)
1290-1393: 🏗️ Heavy liftExtract shared embedding execution flow to prevent drift.
BatchEmbeddingduplicates most ofEmbedding(request setup, send, large-response handling, parse, raw payload handling). This is already diverging in small details and will get harder to maintain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/gemini.go` around lines 1290 - 1393, BatchEmbedding duplicates the full request/response lifecycle from Embedding; extract the common setup, send, large-response handling, and response parsing into a shared helper to avoid drift. Create a helper on GeminiProvider (e.g., executeEmbeddingRequest or handleEmbeddingFlow) that accepts the prepared jsonData, endpoint path ("/models/<model>:embedContents" vs ":batchEmbedContents"), expected response target type (single vs batch struct), model name, and flags for raw request/response sending; move code that sets headers, applies large payload logic (ApplyLargePayloadRequestBody/DrainLargePayloadRemainder), makes the request (PrepareResponseStreaming/MakeRequestWithContext), finalizes response (FinalizeResponseWithLargeDetection), parses via HandleProviderResponse, and builds the BifrostEmbeddingResponse/ExtraFields into that helper; then call this helper from BatchEmbedding and Embedding, keeping model-specific conversion functions (ToGeminiBatchEmbeddingRequest, ToGeminiEmbeddingRequest, ToBifrostEmbeddingResponse) in the callers.core/providers/bedrock/embedding.go (1)
101-106: 💤 Low valueConsider that propagating error details would require changing the converter signature.
When
cohere.ToCohereEmbeddingRequestreturnsnil, the error message "failed to convert to Cohere embedding request" lacks the specific reason for failure. However, the Cohere converter is designed to return only a pointer (not(pointer, error)), which prevents error propagation at the wrapper layer. Internal validation errors within the converter are intentionally swallowed and converted tonil. To expose specific failure reasons, the converter's signature would need to change from(*schemas.BifrostEmbeddingRequest) *CohereEmbeddingRequestto(*schemas.BifrostEmbeddingRequest) (*CohereEmbeddingRequest, error), which is outside this function's scope and would affect the broader converter API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/bedrock/embedding.go` around lines 101 - 106, The current nil-check on cohere.ToCohereEmbeddingRequest(bifrostReq) hides specific conversion errors; change the converter signature from func ToCohereEmbeddingRequest(*schemas.BifrostEmbeddingRequest) *CohereEmbeddingRequest to func ToCohereEmbeddingRequest(*schemas.BifrostEmbeddingRequest) (*CohereEmbeddingRequest, error), update all call sites, and here (in embedding.go) capture the returned (cohereReq, err), return nil, err when err != nil instead of the generic fmt.Errorf message so specific failure reasons propagate; update references to cohereReq and any related tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/internal/llmtests/embedding_multimodal.go`:
- Around line 106-110: The MultimodalEmbedding test sends BatchText (which
builds a single request with three EmbeddingContent entries) unguarded and will
fail for Gemini Vertex models; move the vertexNoBatch guard so BatchText is
skipped for Vertex Gemini (or derive skip from MultimodalEmbeddingModel) before
calling BatchText inside the run closure. Specifically, update the
t.Run("MultimodalEmbedding", ...) logic that defines vertexNoBatch and the run
func to check vertexNoBatch (or model-based condition using
MultimodalEmbeddingModel/ToVertexGeminiEmbeddingRequest()) and skip BatchText
the same way BatchImages and BatchMultimodal are already gated.
In `@core/providers/gemini/gemini.go`:
- Around line 1342-1343: The debug log is writing the full upstream response
body (provider.logger.Debug(fmt.Sprintf("error from %s provider: %s",
providerName, string(resp.Body())))), which may contain user content; change
this to avoid logging raw resp.Body() — replace it with a sanitized summary such
as response status, providerName, and a truncated or redacted snippet (e.g.,
first N bytes or a fixed message like "<redacted body>") before calling
providerUtils.EnrichError and parseGeminiError, keeping the calls to parsedErr
:= providerUtils.EnrichError(ctx, parseGeminiError(resp), jsonData, nil,
provider.sendBackRawRequest, provider.sendBackRawResponse) intact.
In `@core/providers/vertex/embedding.go`:
- Around line 37-46: The code in the loop over bifrostReq.Input.Contents builds
multi-part text by concatenating parts directly (strings.Builder sb +
sb.WriteString) which collapses boundaries; change the join to insert a
delimiter between parts (e.g., a single space or newline) so tokens don’t
merge—update the loop that iterates parts (the variables sb and part in
embedding.go) to check if sb.Len() > 0 and then append a delimiter
(sb.WriteByte(' ') or sb.WriteString("\n")) before sb.WriteString(*part.Text) so
single-part entries remain unchanged but adjacent parts are separated.
In `@core/schemas/embedding.go`:
- Around line 55-59: The EffectiveParams method currently returns i.Params or
defaultParams directly which overwrites defaults and can cause shared-pointer
races; change BifrostEmbeddingBatchItem.EffectiveParams to create a clone of
defaultParams, deep-copy any map fields (e.g., NetworkConfig.ExtraHeaders and
other maps using maps.Copy), then overlay non-nil/non-zero fields from i.Params
onto that clone and return the clone so per-item mutations cannot bleed across
items; apply the same clone+overlay pattern to the similar code region
referenced (the other EffectiveParams-like logic around the 233-245 area) to
ensure all map fields are copied to avoid concurrent data races.
In `@transports/bifrost-http/integrations/genai.go`:
- Around line 170-191: The non-Gemini fallback flattens bifrostBatchReq (from
geminiReq.ToBifrostBatchEmbeddingRequest) into a single EmbeddingRequest and
drops any per-item overrides; detect per-item overrides on bifrostBatchReq.Items
(e.g., item.Params, item.Fallbacks, or any fields that differ from
bifrostBatchReq.Params/Fallbacks) and handle them explicitly: either return a
clear error rejecting non-Gemini batch requests that contain per-item overrides,
or implement a fan-out that builds one schemas.BifrostRequest per item
(preserving each item.Content plus its Params/Fallbacks) instead of collapsing
to a single EmbeddingRequest; update the branch that constructs and returns
&schemas.BifrostRequest{EmbeddingRequest: ...} to perform this check and take
the chosen action.
---
Outside diff comments:
In `@plugins/logging/main.go`:
- Around line 514-551: Add a new case for schemas.BatchEmbeddingRequest
mirroring the schemas.EmbeddingRequest branch: set initialData.Params =
req.BatchEmbeddingRequest.Params, extract each item's embedding content (reuse
or call the same helper used by EmbeddingRequest, e.g., extractEmbeddingInput
but adapted for batch input), compute totalDataSize by iterating each item's
media parts and redact large media via redactEmbeddingMediaData when threshold
from ctx (schemas.BifrostContextKeyLargePayloadRequestThreshold) is exceeded,
then assign the resulting slice to initialData.EmbeddingInput so batch embedding
requests log their (possibly redacted) inputs.
---
Duplicate comments:
In `@core/providers/openai/types.go`:
- Around line 1118-1161: The PR reintroduces a deprecated legacy embedding wire
model (EmbeddingStruct with EmbeddingStr, EmbeddingArray, Embedding2DArray and
its MarshalJSON/UnmarshalJSON) which conflicts with the current embedding
contract; remove EmbeddingStruct and its custom JSON methods and instead ensure
provider responses use the single canonical wire shape ([]float64) and map into
the existing EmbeddingsByType conversion flow (see EmbeddingsByType in
core/schemas/embedding.go) in the provider converters/adapter functions so all
providers funnel through the same conversion path.
In `@core/providers/vertex/embedding.go`:
- Around line 215-240: The code currently appends one EmbeddingData per modality
(prediction.TextEmbedding, prediction.ImageEmbedding,
prediction.VideoEmbeddings) which violates the 1-content→1-embedding contract;
change the logic in the loop that builds embeddings so each prediction yields
exactly one schemas.EmbeddingData (keep using the embeddings slice,
schemas.EmbeddingData and idx), e.g. if prediction.TextEmbedding is non-empty
choose that as the single embedding, else if prediction.ImageEmbedding is
non-empty choose that, else if prediction.VideoEmbeddings has elements choose
the first VideoEmbedding; construct one EmbeddingData with the selected vector
(using append([]float64(nil), ...)) and increment idx once, or alternatively
return an error when multiple modality vectors are present if you prefer to
reject multimodal responses.
- Around line 134-166: The loop over bifrostReq.Input.Contents currently
overwrites earlier parts in a single VertexEmbeddingInstance; fix by
concatenating repeated text parts and explicitly handling duplicate image/video
parts: when processing part.Type in the loop for each content, if part.Type is
EmbeddingContentPartTypeText append part.Text to instance.Text (with a space or
newline) instead of assigning, and for EmbeddingContentPartTypeImage /
EmbeddingContentPartTypeVideo check if instance.Image or instance.Video is
already set and either (a) return a clear error rejecting multiple image/video
parts in one EmbeddingContent or (b) change VertexEmbeddingInstance to accept
slices (e.g., Images []VertexMultimodalImageInput, Videos
[]VertexMultimodalVideoInput) and append new entries; apply the same payload
validation (Data vs URL and gs:// checks) before appending, and ensure
downstream request building uses the new concatenated/merged fields.
In `@tests/integrations/python/tests/test_cohere.py`:
- Around line 158-196: Both tests run against all embedding providers but rely
on Cohere-only features (output_dimension=512 and int8 embeddings); add
capability guards so they skip when the provider/model doesn't support those
features. In test_06_custom_dimensions_embedding and
test_07_multiple_embedding_types, call or implement capability checks before
getting the client (e.g., a helper like supports_output_dimension(provider,
model) and supports_embedding_type(provider, model, "int8") or reuse existing
provider capability helpers) and call pytest.skip when unsupported; keep the
rest of each test (client = get_provider_cohere_client(...), response =
client.embed(...), and assertions) unchanged and reference the existing symbols
test_06_custom_dimensions_embedding, test_07_multiple_embedding_types,
get_provider_cohere_client, format_provider_model to locate where to insert the
guard.
---
Nitpick comments:
In `@core/providers/bedrock/embedding.go`:
- Around line 101-106: The current nil-check on
cohere.ToCohereEmbeddingRequest(bifrostReq) hides specific conversion errors;
change the converter signature from func
ToCohereEmbeddingRequest(*schemas.BifrostEmbeddingRequest)
*CohereEmbeddingRequest to func
ToCohereEmbeddingRequest(*schemas.BifrostEmbeddingRequest)
(*CohereEmbeddingRequest, error), update all call sites, and here (in
embedding.go) capture the returned (cohereReq, err), return nil, err when err !=
nil instead of the generic fmt.Errorf message so specific failure reasons
propagate; update references to cohereReq and any related tests accordingly.
In `@core/providers/cohere/embedding_test.go`:
- Around line 97-99: Add assertions that validate the actual payload values for
the mapped content blocks in addition to their types: after the existing
assertions on CohereContentBlockTypeText and CohereContentBlockTypeImage, assert
that req.Inputs[0].Content[0] contains the original text payload (e.g., the text
field/value equals the text string used earlier in the test) and that
req.Inputs[0].Content[1] contains the original image payload (e.g., the image
URL or image field/value equals the image string used earlier in the test) so
regressions in mapping are caught.
In `@core/providers/gemini/gemini.go`:
- Around line 1290-1393: BatchEmbedding duplicates the full request/response
lifecycle from Embedding; extract the common setup, send, large-response
handling, and response parsing into a shared helper to avoid drift. Create a
helper on GeminiProvider (e.g., executeEmbeddingRequest or handleEmbeddingFlow)
that accepts the prepared jsonData, endpoint path
("/models/<model>:embedContents" vs ":batchEmbedContents"), expected response
target type (single vs batch struct), model name, and flags for raw
request/response sending; move code that sets headers, applies large payload
logic (ApplyLargePayloadRequestBody/DrainLargePayloadRemainder), makes the
request (PrepareResponseStreaming/MakeRequestWithContext), finalizes response
(FinalizeResponseWithLargeDetection), parses via HandleProviderResponse, and
builds the BifrostEmbeddingResponse/ExtraFields into that helper; then call this
helper from BatchEmbedding and Embedding, keeping model-specific conversion
functions (ToGeminiBatchEmbeddingRequest, ToGeminiEmbeddingRequest,
ToBifrostEmbeddingResponse) in the callers.
In `@plugins/semanticcache/test_utils.go`:
- Around line 553-556: Replace the use of the address operator when constructing
EmbeddingContent parts with the repository helper; instead of using Text: &t in
the loop that builds contents (the schemas.EmbeddingContent literal with Type:
schemas.EmbeddingContentPartTypeText), call bifrost.Ptr(t) to create the pointer
consistently, and add the bifrost import if missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5d5320b-8650-4dc5-8c96-460005d9ba3f
📒 Files selected for processing (78)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/embedding.gocore/internal/llmtests/embedding_multimodal.gocore/internal/llmtests/response_validation.gocore/internal/llmtests/tests.gocore/internal/llmtests/utils.gocore/providers/anthropic/anthropic.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/embedding.gocore/providers/bedrock/embedding_test.gocore/providers/bedrock/invoke.gocore/providers/cerebras/cerebras.gocore/providers/cohere/cohere.gocore/providers/cohere/cohere_test.gocore/providers/cohere/embedding.gocore/providers/cohere/embedding_multimodal_test.gocore/providers/cohere/embedding_test.gocore/providers/cohere/types.gocore/providers/elevenlabs/elevenlabs.gocore/providers/fireworks/fireworks.gocore/providers/fireworks/fireworks_test.gocore/providers/gemini/embedding.gocore/providers/gemini/embedding_multimodal_test.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/types.gocore/providers/groq/groq.gocore/providers/huggingface/embedding.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/huggingface_test.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/mistral.gocore/providers/nebius/nebius.gocore/providers/ollama/ollama.gocore/providers/openai/embedding.gocore/providers/openai/openai.gocore/providers/openai/types.gocore/providers/openrouter/openrouter.gocore/providers/parasail/parasail.gocore/providers/perplexity/perplexity.gocore/providers/replicate/replicate.gocore/providers/runway/runway.gocore/providers/sgl/sgl.gocore/providers/vertex/embedding.gocore/providers/vertex/embedding_multimodal_test.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vertex/vertex_test.gocore/providers/vllm/vllm.gocore/providers/xai/xai.gocore/schemas/bifrost.gocore/schemas/embedding.gocore/schemas/embedding_multimodal_test.gocore/schemas/provider.gocore/schemas/serialization_test.gocore/schemas/trace.goframework/logstore/migrations.goframework/logstore/tables.goframework/tracing/llmspan.goplugins/logging/main.goplugins/logging/utils.goplugins/logging/writer.goplugins/semanticcache/go.modplugins/semanticcache/plugin_core_test.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotests/integrations/python/config.ymltests/integrations/python/tests/test_cohere.pytests/integrations/python/tests/test_google.pytransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/cohere.gotransports/bifrost-http/integrations/genai.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/integrations/router.goui/app/workspace/logs/sheets/logDetailView.tsxui/lib/types/logs.ts
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_core_test.go
✅ Files skipped from review due to trivial changes (11)
- core/providers/cohere/types.go
- plugins/semanticcache/go.mod
- core/providers/huggingface/huggingface_test.go
- core/schemas/embedding_multimodal_test.go
- core/providers/cohere/embedding_multimodal_test.go
- core/schemas/serialization_test.go
- core/providers/gemini/embedding_multimodal_test.go
- ui/lib/types/logs.ts
- core/providers/cohere/cohere_test.go
- core/providers/gemini/gemini_test.go
- core/providers/vertex/embedding_multimodal_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- framework/logstore/tables.go
- ui/app/workspace/logs/sheets/logDetailView.tsx
- core/internal/llmtests/account.go
- core/providers/bedrock/invoke.go
- core/internal/llmtests/utils.go
- core/internal/llmtests/embedding.go
- core/providers/gemini/embedding.go
- core/providers/cohere/embedding.go
| t.Run("MultimodalEmbedding", func(t *testing.T) { | ||
| // Vertex Gemini path does not support batch for multimodal inputs. | ||
| vertexNoBatch := testConfig.Provider == schemas.Vertex | ||
|
|
||
| run := func(name string, req *schemas.BifrostEmbeddingRequest, wantCount int) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which Vertex multimodal test model is active.
rg -n -C2 'MultimodalEmbeddingModel|gemini-embedding-2-preview|multimodalembedding@001' core/internal/llmtests
# Confirm whether the Vertex Gemini embedding converter rejects batched Contents.
rg -n -C3 'ToVertexGeminiEmbeddingRequest|len\(.*Contents.*\)|embedContent' core/providers/vertexRepository: maximhq/bifrost
Length of output: 9709
🏁 Script executed:
# View the BatchText and guard check sections in embedding_multimodal.go
sed -n '165,185p' core/internal/llmtests/embedding_multimodal.go
echo "---"
sed -n '206,215p' core/internal/llmtests/embedding_multimodal.goRepository: maximhq/bifrost
Length of output: 1127
Move the Vertex batch guard before BatchText.
BatchText sends three separate EmbeddingContent entries in a single request. When MultimodalEmbeddingModel is set to a Gemini embedding model (e.g., gemini-embedding-2-preview), this is routed to ToVertexGeminiEmbeddingRequest(), which explicitly rejects batches with len(bifrostReq.Input.Contents) > 1. The guard at line 208 only protects BatchImages and BatchMultimodal; BatchText runs unguarded at line 170 and would fail for Vertex Gemini configs.
Gate BatchText behind vertexNoBatch, or derive the skip condition from the selected model (which may differ from the provider-wide setting).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/internal/llmtests/embedding_multimodal.go` around lines 106 - 110, The
MultimodalEmbedding test sends BatchText (which builds a single request with
three EmbeddingContent entries) unguarded and will fail for Gemini Vertex
models; move the vertexNoBatch guard so BatchText is skipped for Vertex Gemini
(or derive skip from MultimodalEmbeddingModel) before calling BatchText inside
the run closure. Specifically, update the t.Run("MultimodalEmbedding", ...)
logic that defines vertexNoBatch and the run func to check vertexNoBatch (or
model-based condition using
MultimodalEmbeddingModel/ToVertexGeminiEmbeddingRequest()) and skip BatchText
the same way BatchImages and BatchMultimodal are already gated.
| provider.logger.Debug(fmt.Sprintf("error from %s provider: %s", providerName, string(resp.Body()))) | ||
| parsedErr := providerUtils.EnrichError(ctx, parseGeminiError(resp), jsonData, nil, provider.sendBackRawRequest, provider.sendBackRawResponse) |
There was a problem hiding this comment.
Avoid logging full upstream error bodies.
Line 1342 logs the full provider response body; this can include user content and should not be written to logs directly.
Suggested fix
- provider.logger.Debug(fmt.Sprintf("error from %s provider: %s", providerName, string(resp.Body())))
+ provider.logger.Debug("error from %s provider (status=%d)", providerName, resp.StatusCode())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| provider.logger.Debug(fmt.Sprintf("error from %s provider: %s", providerName, string(resp.Body()))) | |
| parsedErr := providerUtils.EnrichError(ctx, parseGeminiError(resp), jsonData, nil, provider.sendBackRawRequest, provider.sendBackRawResponse) | |
| provider.logger.Debug("error from %s provider (status=%d)", providerName, resp.StatusCode()) | |
| parsedErr := providerUtils.EnrichError(ctx, parseGeminiError(resp), jsonData, nil, provider.sendBackRawRequest, provider.sendBackRawResponse) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/providers/gemini/gemini.go` around lines 1342 - 1343, The debug log is
writing the full upstream response body
(provider.logger.Debug(fmt.Sprintf("error from %s provider: %s", providerName,
string(resp.Body())))), which may contain user content; change this to avoid
logging raw resp.Body() — replace it with a sanitized summary such as response
status, providerName, and a truncated or redacted snippet (e.g., first N bytes
or a fixed message like "<redacted body>") before calling
providerUtils.EnrichError and parseGeminiError, keeping the calls to parsedErr
:= providerUtils.EnrichError(ctx, parseGeminiError(resp), jsonData, nil,
provider.sendBackRawRequest, provider.sendBackRawResponse) intact.
| for _, content := range bifrostReq.Input.Contents { | ||
| // Vertex text embedding expects a single text string per instance; | ||
| // stitch multiple text parts together. | ||
| var sb strings.Builder | ||
| for _, part := range content { | ||
| if part.Type != schemas.EmbeddingContentPartTypeText || part.Text == nil { | ||
| return nil | ||
| } | ||
| sb.WriteString(*part.Text) | ||
| } |
There was a problem hiding this comment.
Preserve boundaries when joining multiple text parts.
This builds "foo" + "bar" as "foobar", which changes the embedding input for valid multi-part content entries. Add a delimiter before appending subsequent parts so adjacent text parts don't collapse into a different token sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/providers/vertex/embedding.go` around lines 37 - 46, The code in the
loop over bifrostReq.Input.Contents builds multi-part text by concatenating
parts directly (strings.Builder sb + sb.WriteString) which collapses boundaries;
change the join to insert a delimiter between parts (e.g., a single space or
newline) so tokens don’t merge—update the loop that iterates parts (the
variables sb and part in embedding.go) to check if sb.Len() > 0 and then append
a delimiter (sb.WriteByte(' ') or sb.WriteString("\n")) before
sb.WriteString(*part.Text) so single-part entries remain unchanged but adjacent
parts are separated.
| func (i *BifrostEmbeddingBatchItem) EffectiveParams(defaultParams *EmbeddingParameters) *EmbeddingParameters { | ||
| if i.Params != nil { | ||
| return i.Params | ||
| } | ||
| return defaultParams |
There was a problem hiding this comment.
Merge item params onto a cloned default instead of returning either pointer verbatim.
If an item sets only one field in Params, this drops the rest of the batch defaults because i.Params replaces defaultParams wholesale. The inherited branch also returns the shared defaultParams pointer, so any per-item mutation of ExtraParams or other fields can bleed across items and race in concurrent batch processing. Build the effective params from a clone of the batch defaults, then overlay the item-level fields.
Suggested fix
+import "maps"
+
// EffectiveParams returns the item-level Params if set, otherwise the batch-level default.
func (i *BifrostEmbeddingBatchItem) EffectiveParams(defaultParams *EmbeddingParameters) *EmbeddingParameters {
- if i.Params != nil {
- return i.Params
- }
- return defaultParams
+ if defaultParams == nil {
+ return i.Params
+ }
+
+ merged := defaultParams.Clone()
+ if i.Params == nil {
+ return merged
+ }
+ if i.Params.EncodingFormat != nil {
+ merged.EncodingFormat = i.Params.EncodingFormat
+ }
+ if i.Params.Dimensions != nil {
+ merged.Dimensions = i.Params.Dimensions
+ }
+ if i.Params.TaskType != nil {
+ merged.TaskType = i.Params.TaskType
+ }
+ if i.Params.Title != nil {
+ merged.Title = i.Params.Title
+ }
+ if i.Params.AutoTruncate != nil {
+ merged.AutoTruncate = i.Params.AutoTruncate
+ }
+ if i.Params.Truncate != nil {
+ merged.Truncate = i.Params.Truncate
+ }
+ if i.Params.MaxTokens != nil {
+ merged.MaxTokens = i.Params.MaxTokens
+ }
+ if len(i.Params.ExtraParams) > 0 {
+ if merged.ExtraParams == nil {
+ merged.ExtraParams = map[string]interface{}{}
+ }
+ maps.Copy(merged.ExtraParams, i.Params.ExtraParams)
+ }
+ return merged
}
+
+func (p *EmbeddingParameters) Clone() *EmbeddingParameters {
+ if p == nil {
+ return nil
+ }
+ clone := *p
+ clone.ExtraParams = maps.Clone(p.ExtraParams)
+ return &clone
+}As per coding guidelines: Use deep-copy (maps.Copy) for NetworkConfig.ExtraHeaders and any map fields in config structs to prevent concurrent request data races.
Also applies to: 233-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/schemas/embedding.go` around lines 55 - 59, The EffectiveParams method
currently returns i.Params or defaultParams directly which overwrites defaults
and can cause shared-pointer races; change
BifrostEmbeddingBatchItem.EffectiveParams to create a clone of defaultParams,
deep-copy any map fields (e.g., NetworkConfig.ExtraHeaders and other maps using
maps.Copy), then overlay non-nil/non-zero fields from i.Params onto that clone
and return the clone so per-item mutations cannot bleed across items; apply the
same clone+overlay pattern to the similar code region referenced (the other
EffectiveParams-like logic around the 233-245 area) to ensure all map fields are
copied to avoid concurrent data races.
| } else if geminiReq, ok := req.(*gemini.GeminiBatchEmbeddingRequest); ok { | ||
| bifrostBatchReq, err := geminiReq.ToBifrostBatchEmbeddingRequest(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Only Gemini supports BatchEmbedding | ||
| if bifrostBatchReq.Provider == schemas.Gemini { | ||
| return &schemas.BifrostRequest{BatchEmbeddingRequest: bifrostBatchReq}, nil | ||
| } | ||
| contents := make([]schemas.EmbeddingContent, 0, len(bifrostBatchReq.Items)) | ||
| for _, item := range bifrostBatchReq.Items { | ||
| contents = append(contents, item.Content) | ||
| } | ||
| return &schemas.BifrostRequest{ | ||
| EmbeddingRequest: &schemas.BifrostEmbeddingRequest{ | ||
| Provider: bifrostBatchReq.Provider, | ||
| Model: bifrostBatchReq.Model, | ||
| Input: &schemas.EmbeddingInput{Contents: contents}, | ||
| Params: bifrostBatchReq.Params, | ||
| Fallbacks: bifrostBatchReq.Fallbacks, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
Don’t silently discard per-item batch settings in the non-Gemini fallback.
When bifrostBatchReq.Provider != schemas.Gemini, this path flattens the batch into a unary EmbeddingRequest and only forwards bifrostBatchReq.Params. Any item-level overrides on the original batch are lost, so the translated request can run with different parameters than the caller sent. Please either reject non-Gemini batch requests that contain per-item overrides or preserve them via an explicit fan-out path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transports/bifrost-http/integrations/genai.go` around lines 170 - 191, The
non-Gemini fallback flattens bifrostBatchReq (from
geminiReq.ToBifrostBatchEmbeddingRequest) into a single EmbeddingRequest and
drops any per-item overrides; detect per-item overrides on bifrostBatchReq.Items
(e.g., item.Params, item.Fallbacks, or any fields that differ from
bifrostBatchReq.Params/Fallbacks) and handle them explicitly: either return a
clear error rejecting non-Gemini batch requests that contain per-item overrides,
or implement a fan-out that builds one schemas.BifrostRequest per item
(preserving each item.Content plus its Params/Fallbacks) instead of collapsing
to a single EmbeddingRequest; update the branch that constructs and returns
&schemas.BifrostRequest{EmbeddingRequest: ...} to perform this check and take
the chosen action.
There was a problem hiding this comment.
This is intentional. For non-Gemini providers, the batch is flattened into a single EmbeddingRequest using the batch-level Params. Item-level overrides are not applicable on this path since the provider doesn't support native batch semantics.
There was a problem hiding this comment.
@TejasGhatte, understood — since non-Gemini providers lack native batch embedding semantics, collapsing to a single EmbeddingRequest with the batch-level Params is the correct fallback. I'll note this design decision for future reviews.
(ミ・ω・ミ)
✏️ Learnings added
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 2559
File: transports/bifrost-http/integrations/genai.go:170-191
Timestamp: 2026-05-07T18:53:51.985Z
Learning: In `transports/bifrost-http/integrations/genai.go`, when a `GeminiBatchEmbeddingRequest` arrives for a non-Gemini provider (i.e., `bifrostBatchReq.Provider != schemas.Gemini`), the batch is intentionally flattened into a single `schemas.BifrostEmbeddingRequest` using only the batch-level `Params` and `Fallbacks`. Item-level overrides (`item.Params`, `item.Fallbacks`) are silently dropped because non-Gemini providers do not support native batch embedding semantics. Do not flag this as a bug or suggest fan-out/per-item preservation for this path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1312
File: framework/modelcatalog/pricing.go:276-426
Timestamp: 2026-01-14T04:40:11.480Z
Learning: In the Bifrost codebase, ImageUsage and other usage types guarantee that TotalTokens is populated (computed as InputTokens + OutputTokens if providers don’t supply TotalTokens). Reviewers can rely on this invariant and should not assume TotalTokens may be missing when input/output tokens exist. When implementing tiering logic or token-based decisions, you can safely use TotalTokens without extra null/zero guards, provided you’re in a context where InputTokens and OutputTokens are present. If a branch might discard tokens, ensure the invariant is preserved or add explicit checks only where the inputs are confirmed to be valid.
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1326
File: plugins/semanticcache/test_utils.go:545-559
Timestamp: 2026-01-14T13:30:28.760Z
Learning: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability. Replace occurrences of &value where a *T is expected with bifrost.Ptr(value) (or an equivalent call) and ensure the function is in scope and used correctly for the target pointer type.
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1326
File: transports/bifrost-http/integrations/genai.go:51-60
Timestamp: 2026-01-15T09:39:17.760Z
Learning: In transports/bifrost-http/integrations/genai.go, when calling geminiReq.ToBifrostImageGenerationRequest(), rely on the contract that it returns a non-nil *schemas.BifrostImageGenerationRequest and does not return an error. Therefore, you can omit nil-checks or error handling for this call. Ensure there are no alternate code paths that could return nil or an error; if such paths exist, document them and handle accordingly.
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 1740
File: transports/bifrost-http/handlers/governance.go:3168-3214
Timestamp: 2026-02-23T07:58:44.087Z
Learning: In this codebase using GORM, models with CreatedAt and UpdatedAt fields of type time.Time tagged with gorm:"autoCreateTime" and gorm:"autoUpdateTime" are populated automatically by GORM on insert/update. Do not manually set them with time.Now(). Remove any manual initialization; rely on GORM's automatic timestamps. This applies to all Go files with GORM models in the repository.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1670
File: core/providers/anthropic/anthropic.go:689-707
Timestamp: 2026-02-24T04:21:32.824Z
Learning: In Go streaming handlers that reuse pooled response objects (e.g., BifrostChatResponse, BifrostResponsesResponse.Response) via ProcessAndSendResponse, do not release them back to their pools while asynchronous readers (PostLLMHook goroutines) may still access them. Releasing between Acquire and use can cause data races and panics when fields are read by the goroutines. Rely on GC after all references are dropped, and apply this safety pattern to all pooled response types passed through ProcessAndSendResponse in streaming contexts. This should be documented and enforced consistently across all relevant Go files.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 1769
File: transports/bifrost-http/integrations/genai.go:608-645
Timestamp: 2026-02-24T22:17:46.731Z
Learning: In transports/bifrost-http/integrations/genai.go and similar request-type detection paths, when large payload mode is active, the LargePayloadHook consumes ctx.RequestBodyStream() and stores it in BifrostContextKeyLargePayloadReader before request-type detection runs. After consumption, ctx.RequestBodyStream() returns nil. Therefore, an early return checking ctx.RequestBodyStream() != nil will only trigger for non-large-payload streaming bodies, and large-payload requests will correctly reach subsequent metadata-based checks. This is the intended control flow and should not be flagged as unreachable code.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1836
File: core/providers/utils/utils.go:1994-2019
Timestamp: 2026-03-01T13:11:33.245Z
Learning: Enforce the repository-wide convention: all object pools must use raw sync.Pool (not pool.New[T]() or generic pool builders). When reviewing any Go files, verify that pooling code uses sync.Pool directly and constistent with the examples in maximhq/bifrost (e.g., core/bifrost.go, core/providers/anthropic/anthropic.go, core/providers/cohere/cohere.go, core/schemas/plugin.go, framework/tracing/store.go). In particular, do not introduce pool.New[T]() usage; ensure existing pool implementations remain the raw sync.Pool pattern.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1985
File: transports/bifrost-http/integrations/cursor.go:990-1001
Timestamp: 2026-03-12T08:22:11.943Z
Learning: When reviewing OpenAI-related integration routes under transports/bifrost-http/integrations, do not flag the absence of PreCallback: openAILargePayloadPreHook on Responses API routes (e.g., cursor/router openai responses routes). This is an intentional, consistent design choice: only chat-completions-style routes carry the openAILargePayloadPreHook, while the Responses API routes do not.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2152
File: framework/logstore/tables.go:422-431
Timestamp: 2026-03-18T09:04:27.884Z
Learning: Do not flag usages of new(expr) in Go code as compile-time errors. Starting with Go 1.26, new() accepts an expression operand (e.g., new(string(data))), and is valid syntax. Reviewers should only flag actual invalid uses per the Go version used in CI, and assume new(expr) forms are allowed across Go files.
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 2266
File: transports/bifrost-http/integrations/bedrock.go:199-245
Timestamp: 2026-03-26T07:20:02.096Z
Learning: In the Bifrost HTTP integration route configs (e.g., files under transports/bifrost-http/integrations/), RouteConfig should not be expected to define separate ImageEditResponseConverter or ImageVariationResponseConverter fields. The framework routes ImageEdit and ImageVariation responses through the single ImageGenerationResponseConverter (the same field used for image generation). During review of Bedrock or other integration RouteConfigs, do not flag missing dedicated edit/variation response converters as an error when ImageGenerationResponseConverter is present.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2322
File: core/providers/anthropic/responses.go:1988-1992
Timestamp: 2026-03-27T09:20:29.538Z
Learning: In maximhq/bifrost, `BifrostContext` (`core/schemas/context.go`) is a mutable shared context. Its `(*BifrostContext).SetValue(key, value any)` is a pointer-receiver that mutates the internal `userValues map[any]any` in place under a write mutex; it does not create a derived context like Go’s `context.WithValue`. Therefore, when reviewing code, do not flag `SetValue` usage as failing to “propagate” context—subsequent `ctx.Value()` reads on the same `*BifrostContext` pointer should see the updated value immediately.
Learnt from: roroghost17
Repo: maximhq/bifrost PR: 2375
File: transports/bifrost-http/handlers/inference.go:520-528
Timestamp: 2026-03-30T10:05:39.075Z
Learning: For Go code under `transports/bifrost-http/`, treat `TransportInterceptorMiddleware`’s fastHTTP context propagation as intentional: `getBifrostContextFromFastHTTP` stores a `*schemas.BifrostContext` pointer in `FastHTTPUserValueBifrostContext` on the fasthttp `RequestCtx`, plugin hooks (e.g., `HTTPTransportPreHook`) may mutate that pointer in-place via `SetValue`, and downstream handlers should reuse the same pointer via `ConvertToBifrostContext`. Do not flag this as “broken context propagation” or “lost updates” as long as the same pointer is reused across middleware/handlers; the shared pointer is what makes in-place mutations (e.g., `BifrostContextKeyStreamRequest`) visible to downstream logic like `effectiveStream`.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2339
File: plugins/logging/utils.go:531-543
Timestamp: 2026-03-31T05:42:40.984Z
Learning: When reviewing Go code that uses `schemas.ResponsesMessageContent` (as in `plugins/logging/utils.go`), treat `ContentStr` and `ContentBlocks` as mutually exclusive content sources. The type’s `MarshalJSON` enforces that: if `ContentStr != nil`, it is the sole content source and code should not include or suggest a fallback-to-`ContentBlocks` guard when `ContentStr` is non-nil (even if it might be empty). Conversely, only use `ContentBlocks` when `ContentStr` is nil, per the schema contract.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2339
File: plugins/logging/main.go:740-744
Timestamp: 2026-04-03T10:46:22.677Z
Learning: In maximhq/bifrost, Bifrost context key/value assignments are done with `(*BifrostContext).SetValue(key, value)`, not with `context.WithValue`. During code review/searches for where `BifrostContextKey*` constants are set, look for `.SetValue(` patterns (e.g., `rg -n 'SetValue.*BifrostContextKey'`) rather than `context.WithValue`/`WithValue.*Key` to avoid false findings that a key is never set.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2510
File: framework/objectstore/gcs.go:32-43
Timestamp: 2026-04-04T10:05:42.632Z
Learning: In maximhq/bifrost, `schemas.EnvVar` does not implement `IsDefined()`. Reviewers should not flag or suggest calling `schemas.EnvVar.IsDefined()` anywhere in the repository. To check whether an EnvVar config is both present and resolved, use `cfg.Field != nil && cfg.Field.GetValue() != ""` (where `cfg.Field` is the `*schemas.EnvVar` pointer field being evaluated).
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2509
File: framework/logstore/store.go:110-127
Timestamp: 2026-04-04T10:30:13.550Z
Learning: In maximhq/bifrost, do not reference `EnvVar.IsDefined()` (it does not exist on `schemas.EnvVar`). To validate a non-pointer `schemas.EnvVar` field, check `field.GetValue() == ""` (for “defined” it should be non-empty). For pointer `*schemas.EnvVar` fields, use `field != nil && field.GetValue() != ""` to avoid nil dereferences. This rule should be applied across all Go source files in the repo.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2341
File: transports/bifrost-http/handlers/webrtc_realtime.go:1045-1066
Timestamp: 2026-04-07T10:37:38.913Z
Learning: In this repository’s Go code, treat governance context keys as limited to the ones that are actually propagated as request-context values: VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys, and PluginName. Do not suggest adding or flagging missing propagation of BifrostContextKeyGovernanceBusinessUnitID or BifrostContextKeyGovernanceBusinessUnitName in any Go context copy/list. These Business Unit keys are UI-only and appear exclusively in TSX, not in Go request-context propagation (including core/schemas/bifrost.go and bifrost-enterprise/).
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 2605
File: framework/vectorstore/redis.go:1670-1686
Timestamp: 2026-04-09T19:27:39.791Z
Learning: In maximhq/bifrost, the method `(*schemas.EnvVar).CoerceBool(defaultValue bool)` is nil-receiver safe: it begins with `if e == nil { return defaultValue }`, so calling it on an optional `*schemas.EnvVar` (i.e., `nil` possible) should not be treated as a potential nil-dereference panic. During review, do not flag unconditional calls to `.CoerceBool(...)` on optional/nullable `*schemas.EnvVar` fields as nil-deref issues, since the implementation guards against `e == nil` in `core/schemas/envvar.go`.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 2893
File: transports/config.schema.json:1548-1550
Timestamp: 2026-04-21T12:58:33.892Z
Learning: In the maximhq/bifrost public repository, `access_profiles` / `AccessProfiles` is an enterprise-only feature implemented in the private enterprise codebase. During code review of the public repo, do not flag issues like “missing Go struct fields” (e.g., in `ConfigData`, `GovernanceConfig`, or related types) or related unmarshaling/handling gaps specifically for `access_profiles`, since the corresponding fields and runtime behavior are not present in the public code.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 2935
File: transports/bifrost-http/integrations/pydanticai.go:49-54
Timestamp: 2026-04-22T13:14:01.847Z
Learning: When reviewing Go code in maximhq/bifrost, do not flag `resp.WithDefaults()` as a potential nil-pointer panic if `resp` is a `*schemas.BifrostResponsesResponse`. The method `(*schemas.BifrostResponsesResponse).WithDefaults()` (in `core/schemas/responses.go`) is nil-receiver safe: it immediately returns `nil` when `resp == nil`, so calls do not panic even without a prior nil check.
Learnt from: roroghost17
Repo: maximhq/bifrost PR: 2937
File: core/providers/anthropic/request_builder.go:1-1
Timestamp: 2026-04-23T11:26:47.834Z
Learning: In maximhq/bifrost, underscores in non-test Go filenames are an established naming convention (e.g., `count_tokens.go`, `large_payload.go`, `request_builder.go`). During code review, do not flag underscore-containing Go filenames as a naming violation or suggest renaming them. This exception applies only to non-test `.go` files; the general rule may still apply to test files if a separate convention exists.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3067
File: transports/bifrost-http/handlers/inference.go:68-82
Timestamp: 2026-04-27T09:47:28.613Z
Learning: In maximhq/bifrost, `(*ModelCatalog).GetProvidersForModel(model)` (implemented in `framework/modelcatalog/models.go`) already returns only providers that are configured/deployed for the current deployment. Its `modelPool` is populated exclusively via `UpsertModelDataForProvider` for providers registered in the current deployment, and the cross-provider special-case additions (OpenRouter, Vertex, Groq, Bedrock) also gate on `mc.modelPool[provider]`. Therefore, when reviewing code at any call site (e.g., `resolveModelAndProvider` in `transports/bifrost-http/handlers/inference.go`), do not recommend adding an extra filter such as `config.GetAvailableProviders()` or similar to “re-check” availability—doing so would be redundant given the catalog’s implicit invariant.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 3055
File: transports/bifrost-http/integrations/anthropic.go:0-0
Timestamp: 2026-04-27T11:47:36.773Z
Learning: In maximhq/bifrost, keep beta-header filtering split into two layers across provider integrations under transports/bifrost-http/integrations/: (1) The integration-level helper (e.g., extractPassthroughHeaders in transports/bifrost-http/integrations/anthropic.go) must remain provider-agnostic: only whitelist safe/passthrough beta headers (e.g., anthropic-beta) and store them in BifrostContextKeyExtraHeaders; it should not perform provider-specific prefix/feature gating. (2) Provider-aware filtering must occur at the provider wire layer: MergeBetaHeaders(ctx, extraHeaders) reads BifrostContextKeyExtraHeaders, then FilterBetaHeadersForProvider(..., provider, networkConfig.BetaHeaderOverrides) applies prefix→feature gates. For example, if a beta feature is disabled for a given provider/network (e.g., Vertex context-management-*), it should be stripped before the request leaves Bifrost. Do not flag provider-agnostic helpers for “missing provider-aware filtering”; adding that same filtering at the integration layer would duplicate the source of truth and bypass networkConfig.BetaHeaderOverrides.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3166
File: framework/configstore/clientconfig.go:318-332
Timestamp: 2026-04-30T13:35:05.360Z
Learning: In maximhq/bifrost, treat `(*schemas.EnvVar).IsSet()` as nil-receiver safe (it returns `false` when the receiver is `nil`, implemented in `core/schemas/envvar.go`). During code review, do NOT flag unconditional calls like `envVar.IsSet()` as potential nil-dereferences when `envVar` is an optional/nullable `*schemas.EnvVar`. Similarly, follow-on accesses are safe when guarded: calls like `x.IsFromEnv()` / `x.GetValue()` or field access inside `if x.IsSet() { ... }` are safe because `IsSet()` returning `true` implies `x` is non-nil.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3166
File: transports/bifrost-http/lib/config.go:3265-3276
Timestamp: 2026-04-30T13:35:17.403Z
Learning: In maximhq/bifrost, `(*schemas.EnvVar).GetValue()` implemented in `core/schemas/envvar.go` is nil-receiver safe (it immediately checks `if e == nil { return "" }`). During code reviews, you can assume calling `.GetValue()` on an optional `*schemas.EnvVar` pointer will not panic, and you should not recommend adding explicit nil guards solely to protect `.GetValue()` calls.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 3230
File: transports/bifrost-http/handlers/oauth2.go:213-224
Timestamp: 2026-05-05T14:00:01.435Z
Learning: In maximhq/bifrost, `schemas.EnvVar` implements `driver.Valuer` via `core/schemas/envvar.go`: `EnvVar.Value()` is guaranteed to return `(driver.Value, error)` where `error` is always `nil` and the `driver.Value` is always a `string` (sourced from either `EnvVar` or `Val`, both `string` fields). Therefore, during Go reviews, do not flag (a) silent error ignores using `_` for the `error` result of `EnvVar.Value()` and do not treat (b) type assertions `v.(string)` on the `EnvVar.Value()` result as unsafe—these operations should be structurally guaranteed to succeed.
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 2559
File: core/schemas/embedding.go:226-232
Timestamp: 2026-05-07T12:46:19.669Z
Learning: In maximhq/bifrost, the live embedding schema no longer contains `EmbeddingStruct` and therefore `EmbeddingArray` / `Embedding2DArray` fields do not exist anywhere. The active representation is `EmbeddingsByType` in `core/schemas/embedding.go` with `Float []float64`, `Int8 []int8`, `Uint8 []uint8`, `Binary []int8`, `Ubinary []uint8`, and `Base64 *string`. During code reviews, do not flag, require handling, or suggest supporting `EmbeddingArray`/`Embedding2DArray` (or `EmbeddingStruct`) anywhere in the codebase. Also do not suggest changing `Binary` to `[]byte` (an alias for `[]uint8`) because Cohere’s binary embeddings include negative signed integers; `Ubinary` should remain `[]uint8` because Cohere’s ubinary embeddings are 0–255 unsigned values.

Summary
This PR introduces comprehensive multimodal embedding support across the Bifrost platform, enabling text, image, audio, video, and file inputs for embedding generation. The implementation standardizes embedding input handling through a new
Contentsstructure while maintaining backward compatibility.Changes
Text/Texts/Embedding/Embeddingsfields with unifiedContentsarray supporting multiple modalitiesEmbeddingContentParttypes for text, image, audio, video, file, and token inputsKey design decisions:
EmbeddingContententry maps to one output embedding, allowing batch processingType of change
Affected areas
How to test
Test multimodal embedding functionality across providers:
Environment variables for multimodal testing:
VERTEX_API_KEY,VERTEX_PROJECT_ID,VERTEX_REGIONfor Vertex multimodal modelsCOHERE_API_KEYfor Cohere multimodal embeddingsGEMINI_API_KEYfor Gemini embedding modelsScreenshots/Recordings
N/A - Backend API changes only
Breaking changes
The new
Contentsstructure is additive. Existing embedding requests usingtext,texts,embedding, orembeddingsfields continue to work through automatic conversion.Related issues
Addresses multimodal embedding support requirements and standardizes embedding input handling across providers.
Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines